[kvm-unit-tests PATCH v2 0/9] KVM: arm64: PMUv3 Event Counter Tests

[kvm-unit-tests PATCH v2 0/9] KVM: arm64: PMUv3 Event Counter Tests

[Eric Auger]

This series implements tests exercising the PMUv3 event counters.
It tests both the 32-bit and 64-bit versions. Overflow interrupts
also are checked. Those tests only are written for arm64.

It allowed to reveal some issues related to SW_INCR implementation
(esp. related to 64-bit implementation), some problems related to
32-bit <-> 64-bit transitions and consistency of enabled states
of odd and event counters (See [1]).

Overflow interrupt testing relies of one patch from Andre
("arm: gic: Provide per-IRQ helper functions") to enable the
PPI 23, coming from "arm: gic: Test SPIs and interrupt groups"
(https://patchwork.kernel.org/cover/11234975/). Drew kindly
provided "arm64: Provide read/write_sysreg_s".

All PMU tests can be launched with:
./run_tests.sh -g pmu
Tests also can be launched individually. For example:
./arm-run arm/pmu.flat -append 'chained-sw-incr'

With KVM:
- chain-promotion and chained-sw-incr are known to be failing.
  [1] proposed a fix.
- On TX2, I have some random failures due to MEM_ACCESS event
  measured with a great disparity. This is not observed on
  other machines I have access to.
With TCG:
- all new tests are skipped

The series can be found at:
https://github.com/eauger/kut/tree/pmu_event_counters_v2

References:
[1] [PATCH 0/4] KVM/ARM: Misc PMU fixes
(https://www.spinics.net/lists/kvm-arm/msg38886.html)

History:
- Took into account Andre's comments except I did not
  use cnbz in the mem_access_loop() and I did not use
  @loop directly. Those changes had side effects I
  cannot explain on the tests. Anyway I think this can
  be improved later on.
- removed [kvm-unit-tests PATCH 09/10] arm/arm64: gic:
  Introduce setup_irq() helper

RFC -> v1:
- Use new report() proto
- Style cleanup
- do not warn about ARM spec recommendations
- add a comment about PMCEID0/1 splits

Andre Przywara (1):
  arm: gic: Provide per-IRQ helper functions

Andrew Jones (1):
  arm64: Provide read/write_sysreg_s

Eric Auger (7):
  arm: pmu: Let pmu tests take a sub-test parameter
  arm: pmu: Add a pmu struct
  arm: pmu: Check Required Event Support
  arm: pmu: Basic event counter Tests
  arm: pmu: Test chained counter
  arm: pmu: test 32-bit <-> 64-bit transitions
  arm: pmu: Test overflow interrupts

 arm/pmu.c              | 786 ++++++++++++++++++++++++++++++++++++++++-
 arm/unittests.cfg      |  55 ++-
 lib/arm/asm/gic-v3.h   |   2 +
 lib/arm/asm/gic.h      |   9 +
 lib/arm/gic.c          |  90 +++++
 lib/arm64/asm/sysreg.h |  11 +
 6 files changed, 936 insertions(+), 17 deletions(-)

-- 
2.20.1

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[kvm-unit-tests PATCH v2 1/9] arm64: Provide read/write_sysreg_s

[Eric Auger]

From: Andrew Jones <drjones@redhat.com>

Sometimes we need to test access to system registers which are
missing assembler mnemonics.

Signed-off-by: Andrew Jones <drjones@redhat.com>
Reviewed-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
 lib/arm64/asm/sysreg.h | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/lib/arm64/asm/sysreg.h b/lib/arm64/asm/sysreg.h
index a03830b..a45eebd 100644
--- a/lib/arm64/asm/sysreg.h
+++ b/lib/arm64/asm/sysreg.h
@@ -38,6 +38,17 @@
 	asm volatile("msr " xstr(r) ", %x0" : : "rZ" (__val));	\
 } while (0)
 
+#define read_sysreg_s(r) ({					\
+	u64 __val;						\
+	asm volatile("mrs_s %0, " xstr(r) : "=r" (__val));	\
+	__val;							\
+})
+
+#define write_sysreg_s(v, r) do {				\
+	u64 __val = (u64)v;					\
+	asm volatile("msr_s " xstr(r) ", %x0" : : "rZ" (__val));\
+} while (0)
+
 asm(
 "	.irp	num,0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,24,25,26,27,28,29,30\n"
 "	.equ	.L__reg_num_x\\num, \\num\n"
-- 
2.20.1

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[kvm-unit-tests PATCH v2 2/9] arm: pmu: Let pmu tests take a sub-test parameter

[Eric Auger]

As we intend to introduce more PMU tests, let's add
a sub-test parameter that will allow to categorize
them. Existing tests are in the cycle-counter category.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 arm/pmu.c         | 24 +++++++++++++++---------
 arm/unittests.cfg |  7 ++++---
 2 files changed, 19 insertions(+), 12 deletions(-)

diff --git a/arm/pmu.c b/arm/pmu.c
index d5a03a6..e5e012d 100644
--- a/arm/pmu.c
+++ b/arm/pmu.c
@@ -287,22 +287,28 @@ int main(int argc, char *argv[])
 {
 	int cpi = 0;
 
-	if (argc > 1)
-		cpi = atol(argv[1]);
-
 	if (!pmu_probe()) {
 		printf("No PMU found, test skipped...\n");
 		return report_summary();
 	}
 
+	if (argc < 2)
+		report_abort("no test specified");
+
 	report_prefix_push("pmu");
 
-	report(check_pmcr(), "Control register");
-	report(check_cycles_increase(),
-	       "Monotonically increasing cycle count");
-	report(check_cpi(cpi), "Cycle/instruction ratio");
-
-	pmccntr64_test();
+	if (strcmp(argv[1], "cycle-counter") == 0) {
+		report_prefix_push(argv[1]);
+		if (argc > 2)
+			cpi = atol(argv[2]);
+		report(check_pmcr(), "Control register");
+		report(check_cycles_increase(),
+		       "Monotonically increasing cycle count");
+		report(check_cpi(cpi), "Cycle/instruction ratio");
+		pmccntr64_test();
+	} else {
+		report_abort("Unknown sub-test '%s'", argv[1]);
+	}
 
 	return report_summary();
 }
diff --git a/arm/unittests.cfg b/arm/unittests.cfg
index daeb5a0..79f0d7a 100644
--- a/arm/unittests.cfg
+++ b/arm/unittests.cfg
@@ -61,21 +61,22 @@ file = pci-test.flat
 groups = pci
 
 # Test PMU support
-[pmu]
+[pmu-cycle-counter]
 file = pmu.flat
 groups = pmu
+extra_params = -append 'cycle-counter 0'
 
 # Test PMU support (TCG) with -icount IPC=1
 #[pmu-tcg-icount-1]
 #file = pmu.flat
-#extra_params = -icount 0 -append '1'
+#extra_params = -icount 0 -append 'cycle-counter 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'
+#extra_params = -icount 8 -append 'cycle-counter 256'
 #groups = pmu
 #accel = tcg
 
-- 
2.20.1

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [kvm-unit-tests PATCH v2 2/9] arm: pmu: Let pmu tests take a sub-test parameter

[Andre Przywara]

On Thu, 30 Jan 2020 12:25:03 +0100
Eric Auger <eric.auger@redhat.com> wrote:

> As we intend to introduce more PMU tests, let's add
> a sub-test parameter that will allow to categorize
> them. Existing tests are in the cycle-counter category.
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>

Did you change anything? Or just forgot to add my previous R-b?

Anyway,

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Cheers,
Andre

> ---
>  arm/pmu.c         | 24 +++++++++++++++---------
>  arm/unittests.cfg |  7 ++++---
>  2 files changed, 19 insertions(+), 12 deletions(-)
> 
> diff --git a/arm/pmu.c b/arm/pmu.c
> index d5a03a6..e5e012d 100644
> --- a/arm/pmu.c
> +++ b/arm/pmu.c
> @@ -287,22 +287,28 @@ int main(int argc, char *argv[])
>  {
>  	int cpi = 0;
>  
> -	if (argc > 1)
> -		cpi = atol(argv[1]);
> -
>  	if (!pmu_probe()) {
>  		printf("No PMU found, test skipped...\n");
>  		return report_summary();
>  	}
>  
> +	if (argc < 2)
> +		report_abort("no test specified");
> +
>  	report_prefix_push("pmu");
>  
> -	report(check_pmcr(), "Control register");
> -	report(check_cycles_increase(),
> -	       "Monotonically increasing cycle count");
> -	report(check_cpi(cpi), "Cycle/instruction ratio");
> -
> -	pmccntr64_test();
> +	if (strcmp(argv[1], "cycle-counter") == 0) {
> +		report_prefix_push(argv[1]);
> +		if (argc > 2)
> +			cpi = atol(argv[2]);
> +		report(check_pmcr(), "Control register");
> +		report(check_cycles_increase(),
> +		       "Monotonically increasing cycle count");
> +		report(check_cpi(cpi), "Cycle/instruction ratio");
> +		pmccntr64_test();
> +	} else {
> +		report_abort("Unknown sub-test '%s'", argv[1]);
> +	}
>  
>  	return report_summary();
>  }
> diff --git a/arm/unittests.cfg b/arm/unittests.cfg
> index daeb5a0..79f0d7a 100644
> --- a/arm/unittests.cfg
> +++ b/arm/unittests.cfg
> @@ -61,21 +61,22 @@ file = pci-test.flat
>  groups = pci
>  
>  # Test PMU support
> -[pmu]
> +[pmu-cycle-counter]
>  file = pmu.flat
>  groups = pmu
> +extra_params = -append 'cycle-counter 0'
>  
>  # Test PMU support (TCG) with -icount IPC=1
>  #[pmu-tcg-icount-1]
>  #file = pmu.flat
> -#extra_params = -icount 0 -append '1'
> +#extra_params = -icount 0 -append 'cycle-counter 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'
> +#extra_params = -icount 8 -append 'cycle-counter 256'
>  #groups = pmu
>  #accel = tcg
>  

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [kvm-unit-tests PATCH v2 2/9] arm: pmu: Let pmu tests take a sub-test parameter

[Andrew Jones]

On Thu, Jan 30, 2020 at 12:25:03PM +0100, Eric Auger wrote:
> As we intend to introduce more PMU tests, let's add
> a sub-test parameter that will allow to categorize
> them. Existing tests are in the cycle-counter category.
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> ---
>  arm/pmu.c         | 24 +++++++++++++++---------
>  arm/unittests.cfg |  7 ++++---
>  2 files changed, 19 insertions(+), 12 deletions(-)
> 
> diff --git a/arm/pmu.c b/arm/pmu.c
> index d5a03a6..e5e012d 100644
> --- a/arm/pmu.c
> +++ b/arm/pmu.c
> @@ -287,22 +287,28 @@ int main(int argc, char *argv[])
>  {
>  	int cpi = 0;
>  
> -	if (argc > 1)
> -		cpi = atol(argv[1]);
> -
>  	if (!pmu_probe()) {
>  		printf("No PMU found, test skipped...\n");
>  		return report_summary();
>  	}
>  
> +	if (argc < 2)
> +		report_abort("no test specified");
> +
>  	report_prefix_push("pmu");
>  
> -	report(check_pmcr(), "Control register");
> -	report(check_cycles_increase(),
> -	       "Monotonically increasing cycle count");
> -	report(check_cpi(cpi), "Cycle/instruction ratio");
> -
> -	pmccntr64_test();
> +	if (strcmp(argv[1], "cycle-counter") == 0) {
> +		report_prefix_push(argv[1]);
> +		if (argc > 2)
> +			cpi = atol(argv[2]);
> +		report(check_pmcr(), "Control register");
> +		report(check_cycles_increase(),
> +		       "Monotonically increasing cycle count");
> +		report(check_cpi(cpi), "Cycle/instruction ratio");
> +		pmccntr64_test();

Could put a report_prefix_pop here to balance things.

> +	} else {
> +		report_abort("Unknown sub-test '%s'", argv[1]);
> +	}
>  
>  	return report_summary();
>  }
> diff --git a/arm/unittests.cfg b/arm/unittests.cfg
> index daeb5a0..79f0d7a 100644
> --- a/arm/unittests.cfg
> +++ b/arm/unittests.cfg
> @@ -61,21 +61,22 @@ file = pci-test.flat
>  groups = pci
>  
>  # Test PMU support
> -[pmu]
> +[pmu-cycle-counter]
>  file = pmu.flat
>  groups = pmu
> +extra_params = -append 'cycle-counter 0'
>  
>  # Test PMU support (TCG) with -icount IPC=1
>  #[pmu-tcg-icount-1]
>  #file = pmu.flat
> -#extra_params = -icount 0 -append '1'
> +#extra_params = -icount 0 -append 'cycle-counter 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'
> +#extra_params = -icount 8 -append 'cycle-counter 256'
>  #groups = pmu
>  #accel = tcg
>  
> -- 
> 2.20.1
> 
> 

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[kvm-unit-tests PATCH v2 3/9] arm: pmu: Add a pmu struct

[Eric Auger]

This struct aims at storing information potentially used by
all tests such as the pmu version, the read-only part of the
PMCR, the number of implemented event counters, ...

Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 arm/pmu.c | 30 +++++++++++++++++++++++++-----
 1 file changed, 25 insertions(+), 5 deletions(-)

diff --git a/arm/pmu.c b/arm/pmu.c
index e5e012d..d24857e 100644
--- a/arm/pmu.c
+++ b/arm/pmu.c
@@ -33,7 +33,14 @@
 
 #define NR_SAMPLES 10
 
-static unsigned int pmu_version;
+struct pmu {
+	unsigned int version;
+	unsigned int nb_implemented_counters;
+	uint32_t pmcr_ro;
+};
+
+static struct pmu pmu;
+
 #if defined(__arm__)
 #define ID_DFR0_PERFMON_SHIFT 24
 #define ID_DFR0_PERFMON_MASK  0xf
@@ -265,7 +272,7 @@ static bool check_cpi(int cpi)
 static void pmccntr64_test(void)
 {
 #ifdef __arm__
-	if (pmu_version == 0x3) {
+	if (pmu.version == 0x3) {
 		if (ERRATA(9e3f7a296940)) {
 			write_sysreg(0xdead, PMCCNTR64);
 			report(read_sysreg(PMCCNTR64) == 0xdead, "pmccntr64");
@@ -278,9 +285,22 @@ static void pmccntr64_test(void)
 /* Return FALSE if no PMU found, otherwise return TRUE */
 static bool pmu_probe(void)
 {
-	pmu_version = get_pmu_version();
-	report_info("PMU version: %d", pmu_version);
-	return pmu_version != 0 && pmu_version != 0xf;
+	uint32_t pmcr;
+
+	pmu.version = get_pmu_version();
+	report_info("PMU version: %d", pmu.version);
+
+	if (pmu.version == 0 || pmu.version == 0xF)
+		return false;
+
+	pmcr = get_pmcr();
+	pmu.pmcr_ro = pmcr & 0xFFFFFF80;
+	pmu.nb_implemented_counters =
+		(pmcr >> PMU_PMCR_N_SHIFT) & PMU_PMCR_N_MASK;
+	report_info("Implements %d event counters",
+		    pmu.nb_implemented_counters);
+
+	return true;
 }
 
 int main(int argc, char *argv[])
-- 
2.20.1

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [kvm-unit-tests PATCH v2 3/9] arm: pmu: Add a pmu struct

[Andre Przywara]

On Thu, 30 Jan 2020 12:25:04 +0100
Eric Auger <eric.auger@redhat.com> wrote:

> This struct aims at storing information potentially used by
> all tests such as the pmu version, the read-only part of the
> PMCR, the number of implemented event counters, ...
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>

As I stated already in v1:

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Cheers,
Andre

> ---
>  arm/pmu.c | 30 +++++++++++++++++++++++++-----
>  1 file changed, 25 insertions(+), 5 deletions(-)
> 
> diff --git a/arm/pmu.c b/arm/pmu.c
> index e5e012d..d24857e 100644
> --- a/arm/pmu.c
> +++ b/arm/pmu.c
> @@ -33,7 +33,14 @@
>  
>  #define NR_SAMPLES 10
>  
> -static unsigned int pmu_version;
> +struct pmu {
> +	unsigned int version;
> +	unsigned int nb_implemented_counters;
> +	uint32_t pmcr_ro;
> +};
> +
> +static struct pmu pmu;
> +
>  #if defined(__arm__)
>  #define ID_DFR0_PERFMON_SHIFT 24
>  #define ID_DFR0_PERFMON_MASK  0xf
> @@ -265,7 +272,7 @@ static bool check_cpi(int cpi)
>  static void pmccntr64_test(void)
>  {
>  #ifdef __arm__
> -	if (pmu_version == 0x3) {
> +	if (pmu.version == 0x3) {
>  		if (ERRATA(9e3f7a296940)) {
>  			write_sysreg(0xdead, PMCCNTR64);
>  			report(read_sysreg(PMCCNTR64) == 0xdead, "pmccntr64");
> @@ -278,9 +285,22 @@ static void pmccntr64_test(void)
>  /* Return FALSE if no PMU found, otherwise return TRUE */
>  static bool pmu_probe(void)
>  {
> -	pmu_version = get_pmu_version();
> -	report_info("PMU version: %d", pmu_version);
> -	return pmu_version != 0 && pmu_version != 0xf;
> +	uint32_t pmcr;
> +
> +	pmu.version = get_pmu_version();
> +	report_info("PMU version: %d", pmu.version);
> +
> +	if (pmu.version == 0 || pmu.version == 0xF)
> +		return false;
> +
> +	pmcr = get_pmcr();
> +	pmu.pmcr_ro = pmcr & 0xFFFFFF80;
> +	pmu.nb_implemented_counters =
> +		(pmcr >> PMU_PMCR_N_SHIFT) & PMU_PMCR_N_MASK;
> +	report_info("Implements %d event counters",
> +		    pmu.nb_implemented_counters);
> +
> +	return true;
>  }
>  
>  int main(int argc, char *argv[])

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [kvm-unit-tests PATCH v2 3/9] arm: pmu: Add a pmu struct

[Auger Eric]

Hi Andre,

On 3/4/20 7:02 PM, Andre Przywara wrote:
> On Thu, 30 Jan 2020 12:25:04 +0100
> Eric Auger <eric.auger@redhat.com> wrote:
> 
>> This struct aims at storing information potentially used by
>> all tests such as the pmu version, the read-only part of the
>> PMCR, the number of implemented event counters, ...
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> 
> As I stated already in v1:
Hum that's an oversight. Please forgive for the 2 R-b's I have forgotten.

Thanks

Eric
> 
> Reviewed-by: Andre Przywara <andre.przywara@arm.com>


> 
> Cheers,
> Andre
> 
>> ---
>>  arm/pmu.c | 30 +++++++++++++++++++++++++-----
>>  1 file changed, 25 insertions(+), 5 deletions(-)
>>
>> diff --git a/arm/pmu.c b/arm/pmu.c
>> index e5e012d..d24857e 100644
>> --- a/arm/pmu.c
>> +++ b/arm/pmu.c
>> @@ -33,7 +33,14 @@
>>  
>>  #define NR_SAMPLES 10
>>  
>> -static unsigned int pmu_version;
>> +struct pmu {
>> +	unsigned int version;
>> +	unsigned int nb_implemented_counters;
>> +	uint32_t pmcr_ro;
>> +};
>> +
>> +static struct pmu pmu;
>> +
>>  #if defined(__arm__)
>>  #define ID_DFR0_PERFMON_SHIFT 24
>>  #define ID_DFR0_PERFMON_MASK  0xf
>> @@ -265,7 +272,7 @@ static bool check_cpi(int cpi)
>>  static void pmccntr64_test(void)
>>  {
>>  #ifdef __arm__
>> -	if (pmu_version == 0x3) {
>> +	if (pmu.version == 0x3) {
>>  		if (ERRATA(9e3f7a296940)) {
>>  			write_sysreg(0xdead, PMCCNTR64);
>>  			report(read_sysreg(PMCCNTR64) == 0xdead, "pmccntr64");
>> @@ -278,9 +285,22 @@ static void pmccntr64_test(void)
>>  /* Return FALSE if no PMU found, otherwise return TRUE */
>>  static bool pmu_probe(void)
>>  {
>> -	pmu_version = get_pmu_version();
>> -	report_info("PMU version: %d", pmu_version);
>> -	return pmu_version != 0 && pmu_version != 0xf;
>> +	uint32_t pmcr;
>> +
>> +	pmu.version = get_pmu_version();
>> +	report_info("PMU version: %d", pmu.version);
>> +
>> +	if (pmu.version == 0 || pmu.version == 0xF)
>> +		return false;
>> +
>> +	pmcr = get_pmcr();
>> +	pmu.pmcr_ro = pmcr & 0xFFFFFF80;
>> +	pmu.nb_implemented_counters =
>> +		(pmcr >> PMU_PMCR_N_SHIFT) & PMU_PMCR_N_MASK;
>> +	report_info("Implements %d event counters",
>> +		    pmu.nb_implemented_counters);
>> +
>> +	return true;
>>  }
>>  
>>  int main(int argc, char *argv[])
> 

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [kvm-unit-tests PATCH v2 3/9] arm: pmu: Add a pmu struct

[Andrew Jones]

On Thu, Jan 30, 2020 at 12:25:04PM +0100, Eric Auger wrote:
> This struct aims at storing information potentially used by
> all tests such as the pmu version, the read-only part of the
> PMCR, the number of implemented event counters, ...
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> ---
>  arm/pmu.c | 30 +++++++++++++++++++++++++-----
>  1 file changed, 25 insertions(+), 5 deletions(-)
> 
> diff --git a/arm/pmu.c b/arm/pmu.c
> index e5e012d..d24857e 100644
> --- a/arm/pmu.c
> +++ b/arm/pmu.c
> @@ -33,7 +33,14 @@
>  
>  #define NR_SAMPLES 10
>  
> -static unsigned int pmu_version;
> +struct pmu {
> +	unsigned int version;
> +	unsigned int nb_implemented_counters;
> +	uint32_t pmcr_ro;
> +};
> +
> +static struct pmu pmu;
> +
>  #if defined(__arm__)
>  #define ID_DFR0_PERFMON_SHIFT 24
>  #define ID_DFR0_PERFMON_MASK  0xf
> @@ -265,7 +272,7 @@ static bool check_cpi(int cpi)
>  static void pmccntr64_test(void)
>  {
>  #ifdef __arm__
> -	if (pmu_version == 0x3) {
> +	if (pmu.version == 0x3) {
>  		if (ERRATA(9e3f7a296940)) {
>  			write_sysreg(0xdead, PMCCNTR64);
>  			report(read_sysreg(PMCCNTR64) == 0xdead, "pmccntr64");
> @@ -278,9 +285,22 @@ static void pmccntr64_test(void)
>  /* Return FALSE if no PMU found, otherwise return TRUE */
>  static bool pmu_probe(void)
>  {
> -	pmu_version = get_pmu_version();
> -	report_info("PMU version: %d", pmu_version);
> -	return pmu_version != 0 && pmu_version != 0xf;
> +	uint32_t pmcr;
> +
> +	pmu.version = get_pmu_version();
> +	report_info("PMU version: %d", pmu.version);
> +
> +	if (pmu.version == 0 || pmu.version == 0xF)
> +		return false;
> +
> +	pmcr = get_pmcr();
> +	pmu.pmcr_ro = pmcr & 0xFFFFFF80;

I'd prefer we spell out what fields we consider RO. Also this mask doesn't
seem right to me. BIT[7] isn't RO, is it?

> +	pmu.nb_implemented_counters =
> +		(pmcr >> PMU_PMCR_N_SHIFT) & PMU_PMCR_N_MASK;
> +	report_info("Implements %d event counters",
> +		    pmu.nb_implemented_counters);
> +
> +	return true;
>  }
>  
>  int main(int argc, char *argv[])
> -- 
> 2.20.1
> 
> 

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[kvm-unit-tests PATCH v2 4/9] arm: pmu: Check Required Event Support

[Eric Auger]

If event counters are implemented check the common events
required by the PMUv3 are implemented.

Some are unconditionally required (SW_INCR, CPU_CYCLES,
either INST_RETIRED or INST_SPEC). Some others only are
required if the implementation implements some other features.

Check those wich are unconditionally required.

This test currently fails on TCG as neither INST_RETIRED
or INST_SPEC are supported.

Signed-off-by: Eric Auger <eric.auger@redhat.com>

---

v1 -> v2:
- fix is_event_supported()
- fix boolean condition for PMU v4
- fix PMCEID0 definition

RFC ->v1:
- add a comment to explain the PMCEID0/1 splits
---
 arm/pmu.c         | 62 +++++++++++++++++++++++++++++++++++++++++++++++
 arm/unittests.cfg |  6 +++++
 2 files changed, 68 insertions(+)

diff --git a/arm/pmu.c b/arm/pmu.c
index d24857e..4a26a76 100644
--- a/arm/pmu.c
+++ b/arm/pmu.c
@@ -101,6 +101,10 @@ static inline void precise_instrs_loop(int loop, uint32_t pmcr)
 	: [pmcr] "r" (pmcr), [z] "r" (0)
 	: "cc");
 }
+
+/* event counter tests only implemented for aarch64 */
+static void test_event_introspection(void) {}
+
 #elif defined(__aarch64__)
 #define ID_AA64DFR0_PERFMON_SHIFT 8
 #define ID_AA64DFR0_PERFMON_MASK  0xf
@@ -139,6 +143,61 @@ static inline void precise_instrs_loop(int loop, uint32_t pmcr)
 	: [pmcr] "r" (pmcr)
 	: "cc");
 }
+
+#define PMCEID1_EL0 sys_reg(3, 3, 9, 12, 7)
+
+static bool is_event_supported(uint32_t n, bool warn)
+{
+	uint64_t pmceid0 = read_sysreg(pmceid0_el0);
+	uint64_t pmceid1 = read_sysreg_s(PMCEID1_EL0);
+	bool supported;
+	uint64_t reg;
+
+	/*
+	 * The low 32-bits of PMCEID0/1 respectly describe
+	 * event support for events 0-31/32-63. Their High
+	 * 32-bits describe support for extended events
+	 * starting at 0x4000, using the same split.
+	 */
+	if (n >= 0x0  && n <= 0x3F)
+		reg = (pmceid0 & 0xFFFFFFFF) | ((pmceid1 & 0xFFFFFFFF) << 32);
+	else if  (n >= 0x4000 && n <= 0x403F)
+		reg = (pmceid0 >> 32) | ((pmceid1 >> 32) << 32);
+	else
+		abort();
+
+	supported =  reg & (1UL << (n & 0x3F));
+
+	if (!supported && warn)
+		report_info("event %d is not supported", n);
+	return supported;
+}
+
+static void test_event_introspection(void)
+{
+	bool required_events;
+
+	if (!pmu.nb_implemented_counters) {
+		report_skip("No event counter, skip ...");
+		return;
+	}
+
+	/* PMUv3 requires an implementation includes some common events */
+	required_events = is_event_supported(0x0, true) /* SW_INCR */ &&
+			  is_event_supported(0x11, true) /* CPU_CYCLES */ &&
+			  (is_event_supported(0x8, true) /* INST_RETIRED */ ||
+			   is_event_supported(0x1B, true) /* INST_PREC */);
+
+	if (pmu.version == 0x4) {
+		/* ARMv8.1 PMU: STALL_FRONTEND and STALL_BACKEND are required */
+		required_events = required_events &&
+				  is_event_supported(0x23, true) &&
+				  is_event_supported(0x24, true);
+	}
+
+	report(required_events, "Check required events are implemented");
+}
+
 #endif
 
 /*
@@ -326,6 +385,9 @@ int main(int argc, char *argv[])
 		       "Monotonically increasing cycle count");
 		report(check_cpi(cpi), "Cycle/instruction ratio");
 		pmccntr64_test();
+	} else if (strcmp(argv[1], "event-introspection") == 0) {
+		report_prefix_push(argv[1]);
+		test_event_introspection();
 	} else {
 		report_abort("Unknown sub-test '%s'", argv[1]);
 	}
diff --git a/arm/unittests.cfg b/arm/unittests.cfg
index 79f0d7a..4433ef3 100644
--- a/arm/unittests.cfg
+++ b/arm/unittests.cfg
@@ -66,6 +66,12 @@ file = pmu.flat
 groups = pmu
 extra_params = -append 'cycle-counter 0'
 
+[pmu-event-introspection]
+file = pmu.flat
+groups = pmu
+arch = arm64
+extra_params = -append 'event-introspection'
+
 # Test PMU support (TCG) with -icount IPC=1
 #[pmu-tcg-icount-1]
 #file = pmu.flat
-- 
2.20.1

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [kvm-unit-tests PATCH v2 4/9] arm: pmu: Check Required Event Support

[Peter Maydell]

On Thu, 30 Jan 2020 at 11:25, Eric Auger <eric.auger@redhat.com> wrote:
>
> If event counters are implemented check the common events
> required by the PMUv3 are implemented.
>
> Some are unconditionally required (SW_INCR, CPU_CYCLES,
> either INST_RETIRED or INST_SPEC). Some others only are
> required if the implementation implements some other features.
>
> Check those wich are unconditionally required.
>
> This test currently fails on TCG as neither INST_RETIRED
> or INST_SPEC are supported.
>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>
> ---
>

> +static bool is_event_supported(uint32_t n, bool warn)
> +{
> +       uint64_t pmceid0 = read_sysreg(pmceid0_el0);
> +       uint64_t pmceid1 = read_sysreg_s(PMCEID1_EL0);
> +       bool supported;
> +       uint64_t reg;
> +
> +       /*
> +        * The low 32-bits of PMCEID0/1 respectly describe

"respectively"

> +        * event support for events 0-31/32-63. Their High
> +        * 32-bits describe support for extended events
> +        * starting at 0x4000, using the same split.
> +        */
> +       if (n >= 0x0  && n <= 0x3F)
> +               reg = (pmceid0 & 0xFFFFFFFF) | ((pmceid1 & 0xFFFFFFFF) << 32);
> +       else if  (n >= 0x4000 && n <= 0x403F)
> +               reg = (pmceid0 >> 32) | ((pmceid1 >> 32) << 32);
> +       else
> +               abort();
> +
> +       supported =  reg & (1UL << (n & 0x3F));
> +
> +       if (!supported && warn)
> +               report_info("event %d is not supported", n);
> +       return supported;
> +}
> +
> +static void test_event_introspection(void)
> +{
> +       bool required_events;
> +
> +       if (!pmu.nb_implemented_counters) {
> +               report_skip("No event counter, skip ...");
> +               return;
> +       }
> +
> +       /* PMUv3 requires an implementation includes some common events */
> +       required_events = is_event_supported(0x0, true) /* SW_INCR */ &&
> +                         is_event_supported(0x11, true) /* CPU_CYCLES */ &&
> +                         (is_event_supported(0x8, true) /* INST_RETIRED */ ||
> +                          is_event_supported(0x1B, true) /* INST_PREC */);
> +
> +       if (pmu.version == 0x4) {

This condition will only test for v8.1-required events if the PMU
is exactly 8.1, so you lose coverage if the implementation happens
to support ARMv8.4-PMU. Hopefully you have already bailed out
for "ID_AA64DFR0_EL1.PMUVer == 0xf" which means "non-standard IMPDEF
PMU", in which case you can just check >= 0x4.

> +               /* ARMv8.1 PMU: STALL_FRONTEND and STALL_BACKEND are required */
> +               required_events = required_events &&
> +                                 is_event_supported(0x23, true) &&
> +                                 is_event_supported(0x24, true);
> +       }
> +
> +       report(required_events, "Check required events are implemented");
> +}
> +
>  #endif

thanks
-- PMM
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [kvm-unit-tests PATCH v2 4/9] arm: pmu: Check Required Event Support

[Auger Eric]

Hi Peter,

On 2/11/20 4:33 PM, Peter Maydell wrote:
> On Thu, 30 Jan 2020 at 11:25, Eric Auger <eric.auger@redhat.com> wrote:
>>
>> If event counters are implemented check the common events
>> required by the PMUv3 are implemented.
>>
>> Some are unconditionally required (SW_INCR, CPU_CYCLES,
>> either INST_RETIRED or INST_SPEC). Some others only are
>> required if the implementation implements some other features.
>>
>> Check those wich are unconditionally required.
>>
>> This test currently fails on TCG as neither INST_RETIRED
>> or INST_SPEC are supported.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>
>> ---
>>
> 
>> +static bool is_event_supported(uint32_t n, bool warn)
>> +{
>> +       uint64_t pmceid0 = read_sysreg(pmceid0_el0);
>> +       uint64_t pmceid1 = read_sysreg_s(PMCEID1_EL0);
>> +       bool supported;
>> +       uint64_t reg;
>> +
>> +       /*
>> +        * The low 32-bits of PMCEID0/1 respectly describe
> 
> "respectively"
> 
>> +        * event support for events 0-31/32-63. Their High
>> +        * 32-bits describe support for extended events
>> +        * starting at 0x4000, using the same split.
>> +        */
>> +       if (n >= 0x0  && n <= 0x3F)
>> +               reg = (pmceid0 & 0xFFFFFFFF) | ((pmceid1 & 0xFFFFFFFF) << 32);
>> +       else if  (n >= 0x4000 && n <= 0x403F)
>> +               reg = (pmceid0 >> 32) | ((pmceid1 >> 32) << 32);
>> +       else
>> +               abort();
>> +
>> +       supported =  reg & (1UL << (n & 0x3F));
>> +
>> +       if (!supported && warn)
>> +               report_info("event %d is not supported", n);
>> +       return supported;
>> +}
>> +
>> +static void test_event_introspection(void)
>> +{
>> +       bool required_events;
>> +
>> +       if (!pmu.nb_implemented_counters) {
>> +               report_skip("No event counter, skip ...");
>> +               return;
>> +       }
>> +
>> +       /* PMUv3 requires an implementation includes some common events */
>> +       required_events = is_event_supported(0x0, true) /* SW_INCR */ &&
>> +                         is_event_supported(0x11, true) /* CPU_CYCLES */ &&
>> +                         (is_event_supported(0x8, true) /* INST_RETIRED */ ||
>> +                          is_event_supported(0x1B, true) /* INST_PREC */);
>> +
>> +       if (pmu.version == 0x4) {
> 
> This condition will only test for v8.1-required events if the PMU
> is exactly 8.1, so you lose coverage if the implementation happens
> to support ARMv8.4-PMU. Hopefully you have already bailed out
> for "ID_AA64DFR0_EL1.PMUVer == 0xf" which means "non-standard IMPDEF
> PMU", in which case you can just check >= 0x4.
OK thanks

Eric
> 
>> +               /* ARMv8.1 PMU: STALL_FRONTEND and STALL_BACKEND are required */
>> +               required_events = required_events &&
>> +                                 is_event_supported(0x23, true) &&
>> +                                 is_event_supported(0x24, true);
>> +       }
>> +
>> +       report(required_events, "Check required events are implemented");
>> +}
>> +
>>  #endif
> 
> thanks
> -- PMM
> 

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [kvm-unit-tests PATCH v2 4/9] arm: pmu: Check Required Event Support

[Peter Maydell]

On Thu, 30 Jan 2020 at 11:25, Eric Auger <eric.auger@redhat.com> wrote:
>
> If event counters are implemented check the common events
> required by the PMUv3 are implemented.
>
> Some are unconditionally required (SW_INCR, CPU_CYCLES,
> either INST_RETIRED or INST_SPEC). Some others only are
> required if the implementation implements some other features.
>
> Check those wich are unconditionally required.
>
> This test currently fails on TCG as neither INST_RETIRED
> or INST_SPEC are supported.
>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>

> +static bool is_event_supported(uint32_t n, bool warn)
> +{
> +       uint64_t pmceid0 = read_sysreg(pmceid0_el0);
> +       uint64_t pmceid1 = read_sysreg_s(PMCEID1_EL0);
> +       bool supported;
> +       uint64_t reg;
> +
> +       /*
> +        * The low 32-bits of PMCEID0/1 respectly describe
> +        * event support for events 0-31/32-63. Their High
> +        * 32-bits describe support for extended events
> +        * starting at 0x4000, using the same split.
> +        */
> +       if (n >= 0x0  && n <= 0x3F)
> +               reg = (pmceid0 & 0xFFFFFFFF) | ((pmceid1 & 0xFFFFFFFF) << 32);
> +       else if  (n >= 0x4000 && n <= 0x403F)
> +               reg = (pmceid0 >> 32) | ((pmceid1 >> 32) << 32);
> +       else
> +               abort();
> +
> +       supported =  reg & (1UL << (n & 0x3F));
> +
> +       if (!supported && warn)
> +               report_info("event %d is not supported", n);

As with satisfy_prerequisites(), printing this with "0x%x"
would probably be more helpful to most users.

thanks
-- PMM
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [kvm-unit-tests PATCH v2 4/9] arm: pmu: Check Required Event Support

[Auger Eric]

Hi Peter,

On 2/11/20 5:28 PM, Peter Maydell wrote:
> On Thu, 30 Jan 2020 at 11:25, Eric Auger <eric.auger@redhat.com> wrote:
>>
>> If event counters are implemented check the common events
>> required by the PMUv3 are implemented.
>>
>> Some are unconditionally required (SW_INCR, CPU_CYCLES,
>> either INST_RETIRED or INST_SPEC). Some others only are
>> required if the implementation implements some other features.
>>
>> Check those wich are unconditionally required.
>>
>> This test currently fails on TCG as neither INST_RETIRED
>> or INST_SPEC are supported.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>
> 
>> +static bool is_event_supported(uint32_t n, bool warn)
>> +{
>> +       uint64_t pmceid0 = read_sysreg(pmceid0_el0);
>> +       uint64_t pmceid1 = read_sysreg_s(PMCEID1_EL0);
>> +       bool supported;
>> +       uint64_t reg;
>> +
>> +       /*
>> +        * The low 32-bits of PMCEID0/1 respectly describe
>> +        * event support for events 0-31/32-63. Their High
>> +        * 32-bits describe support for extended events
>> +        * starting at 0x4000, using the same split.
>> +        */
>> +       if (n >= 0x0  && n <= 0x3F)
>> +               reg = (pmceid0 & 0xFFFFFFFF) | ((pmceid1 & 0xFFFFFFFF) << 32);
>> +       else if  (n >= 0x4000 && n <= 0x403F)
>> +               reg = (pmceid0 >> 32) | ((pmceid1 >> 32) << 32);
>> +       else
>> +               abort();
>> +
>> +       supported =  reg & (1UL << (n & 0x3F));
>> +
>> +       if (!supported && warn)
>> +               report_info("event %d is not supported", n);
> 
> As with satisfy_prerequisites(), printing this with "0x%x"
> would probably be more helpful to most users.
OK

Thanks

Eric
> 
> thanks
> -- PMM
> 

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [kvm-unit-tests PATCH v2 4/9] arm: pmu: Check Required Event Support

[Andre Przywara]

On Thu, 30 Jan 2020 12:25:05 +0100
Eric Auger <eric.auger@redhat.com> wrote:

Hi,

> If event counters are implemented check the common events
> required by the PMUv3 are implemented.
> 
> Some are unconditionally required (SW_INCR, CPU_CYCLES,
> either INST_RETIRED or INST_SPEC). Some others only are
> required if the implementation implements some other features.
> 
> Check those wich are unconditionally required.
> 
> This test currently fails on TCG as neither INST_RETIRED
> or INST_SPEC are supported.
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> 
> ---
> 
> v1 -> v2:
> - fix is_event_supported()
> - fix boolean condition for PMU v4
> - fix PMCEID0 definition
> 
> RFC ->v1:
> - add a comment to explain the PMCEID0/1 splits
> ---
>  arm/pmu.c         | 62 +++++++++++++++++++++++++++++++++++++++++++++++
>  arm/unittests.cfg |  6 +++++
>  2 files changed, 68 insertions(+)
> 
> diff --git a/arm/pmu.c b/arm/pmu.c
> index d24857e..4a26a76 100644
> --- a/arm/pmu.c
> +++ b/arm/pmu.c
> @@ -101,6 +101,10 @@ static inline void precise_instrs_loop(int loop, uint32_t pmcr)
>  	: [pmcr] "r" (pmcr), [z] "r" (0)
>  	: "cc");
>  }
> +
> +/* event counter tests only implemented for aarch64 */
> +static void test_event_introspection(void) {}
> +
>  #elif defined(__aarch64__)
>  #define ID_AA64DFR0_PERFMON_SHIFT 8
>  #define ID_AA64DFR0_PERFMON_MASK  0xf
> @@ -139,6 +143,61 @@ static inline void precise_instrs_loop(int loop, uint32_t pmcr)
>  	: [pmcr] "r" (pmcr)
>  	: "cc");
>  }
> +
> +#define PMCEID1_EL0 sys_reg(3, 3, 9, 12, 7)
> +
> +static bool is_event_supported(uint32_t n, bool warn)
> +{
> +	uint64_t pmceid0 = read_sysreg(pmceid0_el0);
> +	uint64_t pmceid1 = read_sysreg_s(PMCEID1_EL0);
> +	bool supported;
> +	uint64_t reg;
> +
> +	/*
> +	 * The low 32-bits of PMCEID0/1 respectly describe
> +	 * event support for events 0-31/32-63. Their High
> +	 * 32-bits describe support for extended events
> +	 * starting at 0x4000, using the same split.
> +	 */
> +	if (n >= 0x0  && n <= 0x3F)
> +		reg = (pmceid0 & 0xFFFFFFFF) | ((pmceid1 & 0xFFFFFFFF) << 32);
> +	else if  (n >= 0x4000 && n <= 0x403F)
> +		reg = (pmceid0 >> 32) | ((pmceid1 >> 32) << 32);
> +	else
> +		abort();
> +
> +	supported =  reg & (1UL << (n & 0x3F));
> +
> +	if (!supported && warn)
> +		report_info("event %d is not supported", n);
> +	return supported;
> +}
> +
> +static void test_event_introspection(void)
> +{
> +	bool required_events;
> +
> +	if (!pmu.nb_implemented_counters) {
> +		report_skip("No event counter, skip ...");
> +		return;
> +	}
> +
> +	/* PMUv3 requires an implementation includes some common events */
> +	required_events = is_event_supported(0x0, true) /* SW_INCR */ &&
> +			  is_event_supported(0x11, true) /* CPU_CYCLES */ &&
> +			  (is_event_supported(0x8, true) /* INST_RETIRED */ ||
> +			   is_event_supported(0x1B, true) /* INST_PREC */);
> +
> +	if (pmu.version == 0x4) {

I think this should read >= 0x4, since those requirements are stacked on top of the prevision revision's requirements. Even better with some symbolic name.

With that fixed:

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Cheers,
Andre

> +		/* ARMv8.1 PMU: STALL_FRONTEND and STALL_BACKEND are required */
> +		required_events = required_events &&
> +				  is_event_supported(0x23, true) &&
> +				  is_event_supported(0x24, true);
> +	}
> +
> +	report(required_events, "Check required events are implemented");
> +}
> +
>  #endif
>  
>  /*
> @@ -326,6 +385,9 @@ int main(int argc, char *argv[])
>  		       "Monotonically increasing cycle count");
>  		report(check_cpi(cpi), "Cycle/instruction ratio");
>  		pmccntr64_test();
> +	} else if (strcmp(argv[1], "event-introspection") == 0) {
> +		report_prefix_push(argv[1]);
> +		test_event_introspection();
>  	} else {
>  		report_abort("Unknown sub-test '%s'", argv[1]);
>  	}
> diff --git a/arm/unittests.cfg b/arm/unittests.cfg
> index 79f0d7a..4433ef3 100644
> --- a/arm/unittests.cfg
> +++ b/arm/unittests.cfg
> @@ -66,6 +66,12 @@ file = pmu.flat
>  groups = pmu
>  extra_params = -append 'cycle-counter 0'
>  
> +[pmu-event-introspection]
> +file = pmu.flat
> +groups = pmu
> +arch = arm64
> +extra_params = -append 'event-introspection'
> +
>  # Test PMU support (TCG) with -icount IPC=1
>  #[pmu-tcg-icount-1]
>  #file = pmu.flat

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [kvm-unit-tests PATCH v2 4/9] arm: pmu: Check Required Event Support

[Andrew Jones]

On Thu, Jan 30, 2020 at 12:25:05PM +0100, Eric Auger wrote:
> If event counters are implemented check the common events
> required by the PMUv3 are implemented.
> 
> Some are unconditionally required (SW_INCR, CPU_CYCLES,
> either INST_RETIRED or INST_SPEC). Some others only are
> required if the implementation implements some other features.
> 
> Check those wich are unconditionally required.
> 
> This test currently fails on TCG as neither INST_RETIRED
> or INST_SPEC are supported.
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> 
> ---
> 
> v1 -> v2:
> - fix is_event_supported()
> - fix boolean condition for PMU v4
> - fix PMCEID0 definition
> 
> RFC ->v1:
> - add a comment to explain the PMCEID0/1 splits
> ---
>  arm/pmu.c         | 62 +++++++++++++++++++++++++++++++++++++++++++++++
>  arm/unittests.cfg |  6 +++++
>  2 files changed, 68 insertions(+)
> 
> diff --git a/arm/pmu.c b/arm/pmu.c
> index d24857e..4a26a76 100644
> --- a/arm/pmu.c
> +++ b/arm/pmu.c
> @@ -101,6 +101,10 @@ static inline void precise_instrs_loop(int loop, uint32_t pmcr)
>  	: [pmcr] "r" (pmcr), [z] "r" (0)
>  	: "cc");
>  }
> +
> +/* event counter tests only implemented for aarch64 */
> +static void test_event_introspection(void) {}
> +
>  #elif defined(__aarch64__)
>  #define ID_AA64DFR0_PERFMON_SHIFT 8
>  #define ID_AA64DFR0_PERFMON_MASK  0xf
> @@ -139,6 +143,61 @@ static inline void precise_instrs_loop(int loop, uint32_t pmcr)
>  	: [pmcr] "r" (pmcr)
>  	: "cc");
>  }
> +
> +#define PMCEID1_EL0 sys_reg(3, 3, 9, 12, 7)
> +
> +static bool is_event_supported(uint32_t n, bool warn)
> +{
> +	uint64_t pmceid0 = read_sysreg(pmceid0_el0);
> +	uint64_t pmceid1 = read_sysreg_s(PMCEID1_EL0);
> +	bool supported;
> +	uint64_t reg;
> +
> +	/*
> +	 * The low 32-bits of PMCEID0/1 respectly describe
> +	 * event support for events 0-31/32-63. Their High
> +	 * 32-bits describe support for extended events
> +	 * starting at 0x4000, using the same split.
> +	 */
> +	if (n >= 0x0  && n <= 0x3F)
> +		reg = (pmceid0 & 0xFFFFFFFF) | ((pmceid1 & 0xFFFFFFFF) << 32);

Maybe it's time to add

#define upper_32_bits(n) ((u32)(((n) >> 16) >> 16))
#define lower_32_bits(n) ((u32)(n))

to the kvm-unit-tests framework.

> +	else if  (n >= 0x4000 && n <= 0x403F)
> +		reg = (pmceid0 >> 32) | ((pmceid1 >> 32) << 32);
> +	else
> +		abort();

assert(0) ensure we get a dump_stack() (although I haven't fixed
dump_stack for arm64 yet...). Could also do the assert first

assert((n >= 0x0  && n <= 0x3F) || (n >= 0x4000 && n <= 0x403F))
if (n <= 0x3F)
 ...
else
 ...

What about defines for these hex numbers?


> +
> +	supported =  reg & (1UL << (n & 0x3F));
                   ^ extra space

> +
> +	if (!supported && warn)
> +		report_info("event %d is not supported", n);
> +	return supported;
> +}
> +
> +static void test_event_introspection(void)
> +{
> +	bool required_events;
> +
> +	if (!pmu.nb_implemented_counters) {
> +		report_skip("No event counter, skip ...");
> +		return;
> +	}
> +
> +	/* PMUv3 requires an implementation includes some common events */
> +	required_events = is_event_supported(0x0, true) /* SW_INCR */ &&
> +			  is_event_supported(0x11, true) /* CPU_CYCLES */ &&
> +			  (is_event_supported(0x8, true) /* INST_RETIRED */ ||
> +			   is_event_supported(0x1B, true) /* INST_PREC */);

If defines are created then the comments can go away

	required_events = is_event_supported(SW_INCR, true) &&
			  is_event_supported(CPU_CYCLES, true) &&
			  (is_event_supported(INST_RETIRED, true) ||
			   is_event_supported(INST_PREC, true));


> +
> +	if (pmu.version == 0x4) {
> +		/* ARMv8.1 PMU: STALL_FRONTEND and STALL_BACKEND are required */
> +		required_events = required_events &&
> +				  is_event_supported(0x23, true) &&
> +				  is_event_supported(0x24, true);
> +	}
> +
> +	report(required_events, "Check required events are implemented");
> +}
> +
>  #endif
>  
>  /*
> @@ -326,6 +385,9 @@ int main(int argc, char *argv[])
>  		       "Monotonically increasing cycle count");
>  		report(check_cpi(cpi), "Cycle/instruction ratio");
>  		pmccntr64_test();
> +	} else if (strcmp(argv[1], "event-introspection") == 0) {
> +		report_prefix_push(argv[1]);
> +		test_event_introspection();

prefix pop

>  	} else {
>  		report_abort("Unknown sub-test '%s'", argv[1]);
>  	}
> diff --git a/arm/unittests.cfg b/arm/unittests.cfg
> index 79f0d7a..4433ef3 100644
> --- a/arm/unittests.cfg
> +++ b/arm/unittests.cfg
> @@ -66,6 +66,12 @@ file = pmu.flat
>  groups = pmu
>  extra_params = -append 'cycle-counter 0'
>  
> +[pmu-event-introspection]
> +file = pmu.flat
> +groups = pmu
> +arch = arm64
> +extra_params = -append 'event-introspection'
> +
>  # Test PMU support (TCG) with -icount IPC=1
>  #[pmu-tcg-icount-1]
>  #file = pmu.flat
> -- 
> 2.20.1
> 
>

Thanks,
drew 

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[kvm-unit-tests PATCH v2 5/9] arm: pmu: Basic event counter Tests

[Eric Auger]

Adds the following tests:
- event-counter-config: test event counter configuration
- basic-event-count:
  - programs counters #0 and #1 to count 2 required events
  (resp. CPU_CYCLES and INST_RETIRED). Counter #0 is preset
  to a value close enough to the 32b
  overflow limit so that we check the overflow bit is set
  after the execution of the asm loop.
- mem-access: counts MEM_ACCESS event on counters #0 and #1
  with and without 32-bit overflow.

Signed-off-by: Eric Auger <eric.auger@redhat.com>

---

v1 -> v2:
- fix PMCNTENSET_EL0 and PMCNTENCLR_EL0 op0
- print PMEVTYPER SH
- properly clobber used regs and add "cc"
- simplify mem_access_loop
---
 arm/pmu.c         | 269 ++++++++++++++++++++++++++++++++++++++++++++++
 arm/unittests.cfg |  18 ++++
 2 files changed, 287 insertions(+)

diff --git a/arm/pmu.c b/arm/pmu.c
index 4a26a76..1b0101f 100644
--- a/arm/pmu.c
+++ b/arm/pmu.c
@@ -18,9 +18,15 @@
 #include "asm/barrier.h"
 #include "asm/sysreg.h"
 #include "asm/processor.h"
+#include <bitops.h>
+#include <asm/gic.h>
 
 #define PMU_PMCR_E         (1 << 0)
+#define PMU_PMCR_P         (1 << 1)
 #define PMU_PMCR_C         (1 << 2)
+#define PMU_PMCR_D         (1 << 3)
+#define PMU_PMCR_X         (1 << 4)
+#define PMU_PMCR_DP        (1 << 5)
 #define PMU_PMCR_LC        (1 << 6)
 #define PMU_PMCR_N_SHIFT   11
 #define PMU_PMCR_N_MASK    0x1f
@@ -104,6 +110,9 @@ static inline void precise_instrs_loop(int loop, uint32_t pmcr)
 
 /* event counter tests only implemented for aarch64 */
 static void test_event_introspection(void) {}
+static void test_event_counter_config(void) {}
+static void test_basic_event_count(void) {}
+static void test_mem_access(void) {}
 
 #elif defined(__aarch64__)
 #define ID_AA64DFR0_PERFMON_SHIFT 8
@@ -145,6 +154,33 @@ static inline void precise_instrs_loop(int loop, uint32_t pmcr)
 }
 
 #define PMCEID1_EL0 sys_reg(3, 3, 9, 12, 7)
+#define PMCNTENSET_EL0 sys_reg(3, 3, 9, 12, 1)
+#define PMCNTENCLR_EL0 sys_reg(3, 3, 9, 12, 2)
+
+#define PMEVTYPER_EXCLUDE_EL1 (1 << 31)
+#define PMEVTYPER_EXCLUDE_EL0 (1 << 30)
+
+#define regn_el0(__reg, __n) __reg ## __n  ## _el0
+#define write_regn(__reg, __n, __val) \
+	write_sysreg((__val), __reg ## __n ## _el0)
+
+#define read_regn(__reg, __n) \
+	read_sysreg(__reg ## __n ## _el0)
+
+#define print_pmevtyper(__s, __n) do { \
+	uint32_t val; \
+	val = read_regn(pmevtyper, __n);\
+	report_info("%s pmevtyper%d=0x%x, eventcount=0x%x (p=%ld, u=%ld nsk=%ld, nsu=%ld, nsh=%ld m=%ld, mt=%ld, sh=%ld)", \
+			(__s), (__n), val, val & 0xFFFF,	\
+			(BIT_MASK(31) & val) >> 31,		\
+			(BIT_MASK(30) & val) >> 30,		\
+			(BIT_MASK(29) & val) >> 29,		\
+			(BIT_MASK(28) & val) >> 28,		\
+			(BIT_MASK(27) & val) >> 27,		\
+			(BIT_MASK(26) & val) >> 26,		\
+			(BIT_MASK(25) & val) >> 25);		\
+			(BIT_MASK(24) & val) >> 24);		\
+	} while (0)
 
 static bool is_event_supported(uint32_t n, bool warn)
 {
@@ -198,6 +234,230 @@ static void test_event_introspection(void)
 	report(required_events, "Check required events are implemented");
 }
 
+/*
+ * 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. Loads @loop times the data at @address into x9.
+ */
+static void mem_access_loop(void *addr, int loop, uint32_t pmcr)
+{
+asm volatile(
+	"       msr     pmcr_el0, %[pmcr]\n"
+	"       isb\n"
+	"       mov     x10, %[loop]\n"
+	"1:     sub     x10, x10, #1\n"
+	"       ldr x9, [%[addr]]\n"
+	"       cmp     x10, #0x0\n"
+	"       b.gt    1b\n"
+	"       msr     pmcr_el0, xzr\n"
+	"       isb\n"
+	:
+	: [addr] "r" (addr), [pmcr] "r" (pmcr), [loop] "r" (loop)
+	: "x9", "x10", "cc");
+}
+
+static void pmu_reset(void)
+{
+	/* reset all counters, counting disabled at PMCR level*/
+	set_pmcr(pmu.pmcr_ro | PMU_PMCR_LC | PMU_PMCR_C | PMU_PMCR_P);
+	/* Disable all counters */
+	write_sysreg_s(0xFFFFFFFF, PMCNTENCLR_EL0);
+	/* clear overflow reg */
+	write_sysreg(0xFFFFFFFF, pmovsclr_el0);
+	/* disable overflow interrupts on all counters */
+	write_sysreg(0xFFFFFFFF, pmintenclr_el1);
+	isb();
+}
+
+static void test_event_counter_config(void)
+{
+	int i;
+
+	if (!pmu.nb_implemented_counters) {
+		report_skip("No event counter, skip ...");
+		return;
+	}
+
+	pmu_reset();
+
+	/*
+	 * Test setting through PMESELR/PMXEVTYPER and PMEVTYPERn read,
+	 * select counter 0
+	 */
+	write_sysreg(1, PMSELR_EL0);
+	/* program this counter to count unsupported event */
+	write_sysreg(0xEA, PMXEVTYPER_EL0);
+	write_sysreg(0xdeadbeef, PMXEVCNTR_EL0);
+	report((read_regn(pmevtyper, 1) & 0xFFF) == 0xEA,
+		"PMESELR/PMXEVTYPER/PMEVTYPERn");
+	report((read_regn(pmevcntr, 1) == 0xdeadbeef),
+		"PMESELR/PMXEVCNTR/PMEVCNTRn");
+
+	/* try to configure an unsupported event within the range [0x0, 0x3F] */
+	for (i = 0; i <= 0x3F; i++) {
+		if (!is_event_supported(i, false))
+			break;
+	}
+	if (i > 0x3F) {
+		report_skip("pmevtyper: all events within [0x0, 0x3F] are supported");
+		return;
+	}
+
+	/* select counter 0 */
+	write_sysreg(0, PMSELR_EL0);
+	/* program this counter to count unsupported event */
+	write_sysreg(i, PMXEVCNTR_EL0);
+	/* read the counter value */
+	read_sysreg(PMXEVCNTR_EL0);
+	report(read_sysreg(PMXEVCNTR_EL0) == i,
+		"read of a counter programmed with unsupported event");
+
+}
+
+static bool satisfy_prerequisites(uint32_t *events, unsigned int nb_events)
+{
+	int i;
+
+	if (pmu.nb_implemented_counters < nb_events) {
+		report_skip("Skip test as number of counters is too small (%d)",
+			    pmu.nb_implemented_counters);
+		return false;
+	}
+
+	for (i = 0; i < nb_events; i++) {
+		if (!is_event_supported(events[i], false)) {
+			report_skip("Skip test as event %d is not supported",
+				    events[i]);
+			return false;
+		}
+	}
+	return true;
+}
+
+static void test_basic_event_count(void)
+{
+	uint32_t implemented_counter_mask, non_implemented_counter_mask;
+	uint32_t counter_mask;
+	uint32_t events[] = {
+		0x11,	/* CPU_CYCLES */
+		0x8,	/* INST_RETIRED */
+	};
+
+	if (!satisfy_prerequisites(events, ARRAY_SIZE(events)))
+		return;
+
+	implemented_counter_mask = BIT(pmu.nb_implemented_counters) - 1;
+	non_implemented_counter_mask = ~(BIT(31) | implemented_counter_mask);
+	counter_mask = implemented_counter_mask | non_implemented_counter_mask;
+
+	write_regn(pmevtyper, 0, events[0] | PMEVTYPER_EXCLUDE_EL0);
+	write_regn(pmevtyper, 1, events[1] | PMEVTYPER_EXCLUDE_EL0);
+
+	/* disable all counters */
+	write_sysreg_s(0xFFFFFFFF, PMCNTENCLR_EL0);
+	report(!read_sysreg_s(PMCNTENCLR_EL0) && !read_sysreg_s(PMCNTENSET_EL0),
+		"pmcntenclr: disable all counters");
+
+	/*
+	 * clear cycle and all event counters and allow counter enablement
+	 * through PMCNTENSET. LC is RES1.
+	 */
+	set_pmcr(pmu.pmcr_ro | PMU_PMCR_LC | PMU_PMCR_C | PMU_PMCR_P);
+	isb();
+	report(get_pmcr() == (pmu.pmcr_ro | PMU_PMCR_LC), "pmcr: reset counters");
+
+	/* Preset counter #0 to 0xFFFFFFF0 to trigger an overflow interrupt */
+	write_regn(pmevcntr, 0, 0xFFFFFFF0);
+	report(read_regn(pmevcntr, 0) == 0xFFFFFFF0,
+		"counter #0 preset to 0xFFFFFFF0");
+	report(!read_regn(pmevcntr, 1), "counter #1 is 0");
+
+	/*
+	 * Enable all implemented counters and also attempt to enable
+	 * not supported counters. Counting still is disabled by !PMCR.E
+	 */
+	write_sysreg_s(counter_mask, PMCNTENSET_EL0);
+
+	/* check only those implemented are enabled */
+	report((read_sysreg_s(PMCNTENSET_EL0) == read_sysreg_s(PMCNTENCLR_EL0)) &&
+		(read_sysreg_s(PMCNTENSET_EL0) == implemented_counter_mask),
+		"pmcntenset: enabled implemented_counters");
+
+	/* Disable all counters but counters #0 and #1 */
+	write_sysreg_s(~0x3, PMCNTENCLR_EL0);
+	report((read_sysreg_s(PMCNTENSET_EL0) == read_sysreg_s(PMCNTENCLR_EL0)) &&
+		(read_sysreg_s(PMCNTENSET_EL0) == 0x3),
+		"pmcntenset: just enabled #0 and #1");
+
+	/* clear overflow register */
+	write_sysreg(0xFFFFFFFF, pmovsclr_el0);
+	report(!read_sysreg(pmovsclr_el0), "check overflow reg is 0");
+
+	/* disable overflow interrupts on all counters*/
+	write_sysreg(0xFFFFFFFF, pmintenclr_el1);
+	report(!read_sysreg(pmintenclr_el1),
+		"pmintenclr_el1=0, all interrupts disabled");
+
+	/* enable overflow interrupts on all event counters */
+	write_sysreg(implemented_counter_mask | non_implemented_counter_mask,
+		     pmintenset_el1);
+	report(read_sysreg(pmintenset_el1) == implemented_counter_mask,
+		"overflow interrupts enabled on all implemented counters");
+
+	/* Set PMCR.E, execute asm code and unset PMCR.E */
+	precise_instrs_loop(20, pmu.pmcr_ro | PMU_PMCR_E);
+
+	report_info("counter #0 is 0x%lx (CPU_CYCLES)",
+		    read_regn(pmevcntr, 0));
+	report_info("counter #1 is 0x%lx (INST_RETIRED)",
+		    read_regn(pmevcntr, 1));
+
+	report_info("overflow reg = 0x%lx", read_sysreg(pmovsclr_el0));
+	report(read_sysreg(pmovsclr_el0) & 0x1,
+		"check overflow happened on #0 only");
+}
+
+static void test_mem_access(void)
+{
+	void *addr = malloc(PAGE_SIZE);
+	uint32_t events[] = {
+		0x13,   /* MEM_ACCESS */
+		0x13,   /* MEM_ACCESS */
+	};
+
+	if (!satisfy_prerequisites(events, ARRAY_SIZE(events)))
+		return;
+
+	pmu_reset();
+
+	write_regn(pmevtyper, 0, events[0] | PMEVTYPER_EXCLUDE_EL0);
+	write_regn(pmevtyper, 1, events[1] | PMEVTYPER_EXCLUDE_EL0);
+	write_sysreg_s(0x3, PMCNTENSET_EL0);
+	isb();
+	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E);
+	report_info("counter #0 is %ld (MEM_ACCESS)", read_regn(pmevcntr, 0));
+	report_info("counter #1 is %ld (MEM_ACCESS)", read_regn(pmevcntr, 1));
+	/* We may measure more than 20 mem access depending on the core */
+	report((read_regn(pmevcntr, 0) == read_regn(pmevcntr, 1)) &&
+	       (read_regn(pmevcntr, 0) >= 20) && !read_sysreg(pmovsclr_el0),
+	       "Ran 20 mem accesses");
+
+	pmu_reset();
+
+	write_regn(pmevcntr, 0, 0xFFFFFFFA);
+	write_regn(pmevcntr, 1, 0xFFFFFFF0);
+	write_sysreg_s(0x3, PMCNTENSET_EL0);
+	isb();
+	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E);
+	report(read_sysreg(pmovsclr_el0) == 0x3,
+	       "Ran 20 mem accesses with expected overflows on both counters");
+	report_info("cnt#0 = %ld cnt#1=%ld overflow=0x%lx",
+			read_regn(pmevcntr, 0), read_regn(pmevcntr, 1),
+			read_sysreg(pmovsclr_el0));
+}
+
 #endif
 
 /*
@@ -388,6 +648,15 @@ int main(int argc, char *argv[])
 	} else if (strcmp(argv[1], "event-introspection") == 0) {
 		report_prefix_push(argv[1]);
 		test_event_introspection();
+	} else if (strcmp(argv[1], "event-counter-config") == 0) {
+		report_prefix_push(argv[1]);
+		test_event_counter_config();
+	} else if (strcmp(argv[1], "basic-event-count") == 0) {
+		report_prefix_push(argv[1]);
+		test_basic_event_count();
+	} else if (strcmp(argv[1], "mem-access") == 0) {
+		report_prefix_push(argv[1]);
+		test_mem_access();
 	} else {
 		report_abort("Unknown sub-test '%s'", argv[1]);
 	}
diff --git a/arm/unittests.cfg b/arm/unittests.cfg
index 4433ef3..7a59403 100644
--- a/arm/unittests.cfg
+++ b/arm/unittests.cfg
@@ -72,6 +72,24 @@ groups = pmu
 arch = arm64
 extra_params = -append 'event-introspection'
 
+[pmu-event-counter-config]
+file = pmu.flat
+groups = pmu
+arch = arm64
+extra_params = -append 'event-counter-config'
+
+[pmu-basic-event-count]
+file = pmu.flat
+groups = pmu
+arch = arm64
+extra_params = -append 'basic-event-count'
+
+[pmu-mem-access]
+file = pmu.flat
+groups = pmu
+arch = arm64
+extra_params = -append 'mem-access'
+
 # Test PMU support (TCG) with -icount IPC=1
 #[pmu-tcg-icount-1]
 #file = pmu.flat
-- 
2.20.1

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [kvm-unit-tests PATCH v2 5/9] arm: pmu: Basic event counter Tests

[Peter Maydell]

On Thu, 30 Jan 2020 at 11:26, Eric Auger <eric.auger@redhat.com> wrote:
>
> Adds the following tests:
> - event-counter-config: test event counter configuration
> - basic-event-count:
>   - programs counters #0 and #1 to count 2 required events
>   (resp. CPU_CYCLES and INST_RETIRED). Counter #0 is preset
>   to a value close enough to the 32b
>   overflow limit so that we check the overflow bit is set
>   after the execution of the asm loop.
> - mem-access: counts MEM_ACCESS event on counters #0 and #1
>   with and without 32-bit overflow.
>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>

> +static bool satisfy_prerequisites(uint32_t *events, unsigned int nb_events)
> +{
> +       int i;
> +
> +       if (pmu.nb_implemented_counters < nb_events) {
> +               report_skip("Skip test as number of counters is too small (%d)",
> +                           pmu.nb_implemented_counters);
> +               return false;
> +       }
> +
> +       for (i = 0; i < nb_events; i++) {
> +               if (!is_event_supported(events[i], false)) {
> +                       report_skip("Skip test as event %d is not supported",
> +                                   events[i]);

Event numbers are given in hex in the Arm ARM and also
specified in hex in your test source code. I think it
would be more helpful if the message here used "0x%x", to
save the reader having to do the decimal-to-hex conversion
to find the event in the spec or the test case.

thanks
-- PMM
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [kvm-unit-tests PATCH v2 5/9] arm: pmu: Basic event counter Tests

[Auger Eric]

Hi Peter,

On 2/11/20 5:27 PM, Peter Maydell wrote:
> On Thu, 30 Jan 2020 at 11:26, Eric Auger <eric.auger@redhat.com> wrote:
>>
>> Adds the following tests:
>> - event-counter-config: test event counter configuration
>> - basic-event-count:
>>   - programs counters #0 and #1 to count 2 required events
>>   (resp. CPU_CYCLES and INST_RETIRED). Counter #0 is preset
>>   to a value close enough to the 32b
>>   overflow limit so that we check the overflow bit is set
>>   after the execution of the asm loop.
>> - mem-access: counts MEM_ACCESS event on counters #0 and #1
>>   with and without 32-bit overflow.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> 
>> +static bool satisfy_prerequisites(uint32_t *events, unsigned int nb_events)
>> +{
>> +       int i;
>> +
>> +       if (pmu.nb_implemented_counters < nb_events) {
>> +               report_skip("Skip test as number of counters is too small (%d)",
>> +                           pmu.nb_implemented_counters);
>> +               return false;
>> +       }
>> +
>> +       for (i = 0; i < nb_events; i++) {
>> +               if (!is_event_supported(events[i], false)) {
>> +                       report_skip("Skip test as event %d is not supported",
>> +                                   events[i]);
> 
> Event numbers are given in hex in the Arm ARM and also
> specified in hex in your test source code. I think it
> would be more helpful if the message here used "0x%x", to
> save the reader having to do the decimal-to-hex conversion
> to find the event in the spec or the test case.
Sure I will fix that

Thanks

Eric
> 
> thanks
> -- PMM
> 

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [kvm-unit-tests PATCH v2 5/9] arm: pmu: Basic event counter Tests

[Andre Przywara]

On Thu, 30 Jan 2020 12:25:06 +0100
Eric Auger <eric.auger@redhat.com> wrote:

> Adds the following tests:
> - event-counter-config: test event counter configuration
> - basic-event-count:
>   - programs counters #0 and #1 to count 2 required events
>   (resp. CPU_CYCLES and INST_RETIRED). Counter #0 is preset
>   to a value close enough to the 32b
>   overflow limit so that we check the overflow bit is set
>   after the execution of the asm loop.
> - mem-access: counts MEM_ACCESS event on counters #0 and #1
>   with and without 32-bit overflow.
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> 
> ---
> 
> v1 -> v2:
> - fix PMCNTENSET_EL0 and PMCNTENCLR_EL0 op0
> - print PMEVTYPER SH
> - properly clobber used regs and add "cc"
> - simplify mem_access_loop
> ---
>  arm/pmu.c         | 269 ++++++++++++++++++++++++++++++++++++++++++++++
>  arm/unittests.cfg |  18 ++++
>  2 files changed, 287 insertions(+)
> 
> diff --git a/arm/pmu.c b/arm/pmu.c
> index 4a26a76..1b0101f 100644
> --- a/arm/pmu.c
> +++ b/arm/pmu.c
> @@ -18,9 +18,15 @@
>  #include "asm/barrier.h"
>  #include "asm/sysreg.h"
>  #include "asm/processor.h"
> +#include <bitops.h>
> +#include <asm/gic.h>
>  
>  #define PMU_PMCR_E         (1 << 0)
> +#define PMU_PMCR_P         (1 << 1)
>  #define PMU_PMCR_C         (1 << 2)
> +#define PMU_PMCR_D         (1 << 3)
> +#define PMU_PMCR_X         (1 << 4)
> +#define PMU_PMCR_DP        (1 << 5)
>  #define PMU_PMCR_LC        (1 << 6)
>  #define PMU_PMCR_N_SHIFT   11
>  #define PMU_PMCR_N_MASK    0x1f
> @@ -104,6 +110,9 @@ static inline void precise_instrs_loop(int loop, uint32_t pmcr)
>  
>  /* event counter tests only implemented for aarch64 */
>  static void test_event_introspection(void) {}
> +static void test_event_counter_config(void) {}
> +static void test_basic_event_count(void) {}
> +static void test_mem_access(void) {}
>  
>  #elif defined(__aarch64__)
>  #define ID_AA64DFR0_PERFMON_SHIFT 8
> @@ -145,6 +154,33 @@ static inline void precise_instrs_loop(int loop, uint32_t pmcr)
>  }
>  
>  #define PMCEID1_EL0 sys_reg(3, 3, 9, 12, 7)
> +#define PMCNTENSET_EL0 sys_reg(3, 3, 9, 12, 1)
> +#define PMCNTENCLR_EL0 sys_reg(3, 3, 9, 12, 2)
> +
> +#define PMEVTYPER_EXCLUDE_EL1 (1 << 31)
> +#define PMEVTYPER_EXCLUDE_EL0 (1 << 30)

Please use 1U << or BIT() ;-)

Rest looks OK now:

Reviewed-by: Andre Przywara <andre.przywara@arm.com>


> +
> +#define regn_el0(__reg, __n) __reg ## __n  ## _el0
> +#define write_regn(__reg, __n, __val) \
> +	write_sysreg((__val), __reg ## __n ## _el0)
> +
> +#define read_regn(__reg, __n) \
> +	read_sysreg(__reg ## __n ## _el0)
> +
> +#define print_pmevtyper(__s, __n) do { \
> +	uint32_t val; \
> +	val = read_regn(pmevtyper, __n);\
> +	report_info("%s pmevtyper%d=0x%x, eventcount=0x%x (p=%ld, u=%ld nsk=%ld, nsu=%ld, nsh=%ld m=%ld, mt=%ld, sh=%ld)", \
> +			(__s), (__n), val, val & 0xFFFF,	\
> +			(BIT_MASK(31) & val) >> 31,		\
> +			(BIT_MASK(30) & val) >> 30,		\
> +			(BIT_MASK(29) & val) >> 29,		\
> +			(BIT_MASK(28) & val) >> 28,		\
> +			(BIT_MASK(27) & val) >> 27,		\
> +			(BIT_MASK(26) & val) >> 26,		\
> +			(BIT_MASK(25) & val) >> 25);		\
> +			(BIT_MASK(24) & val) >> 24);		\
> +	} while (0)
>  
>  static bool is_event_supported(uint32_t n, bool warn)
>  {
> @@ -198,6 +234,230 @@ static void test_event_introspection(void)
>  	report(required_events, "Check required events are implemented");
>  }
>  
> +/*
> + * 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. Loads @loop times the data at @address into x9.
> + */
> +static void mem_access_loop(void *addr, int loop, uint32_t pmcr)
> +{
> +asm volatile(
> +	"       msr     pmcr_el0, %[pmcr]\n"
> +	"       isb\n"
> +	"       mov     x10, %[loop]\n"
> +	"1:     sub     x10, x10, #1\n"
> +	"       ldr x9, [%[addr]]\n"
> +	"       cmp     x10, #0x0\n"
> +	"       b.gt    1b\n"
> +	"       msr     pmcr_el0, xzr\n"
> +	"       isb\n"
> +	:
> +	: [addr] "r" (addr), [pmcr] "r" (pmcr), [loop] "r" (loop)
> +	: "x9", "x10", "cc");
> +}
> +
> +static void pmu_reset(void)
> +{
> +	/* reset all counters, counting disabled at PMCR level*/
> +	set_pmcr(pmu.pmcr_ro | PMU_PMCR_LC | PMU_PMCR_C | PMU_PMCR_P);
> +	/* Disable all counters */
> +	write_sysreg_s(0xFFFFFFFF, PMCNTENCLR_EL0);
> +	/* clear overflow reg */
> +	write_sysreg(0xFFFFFFFF, pmovsclr_el0);
> +	/* disable overflow interrupts on all counters */
> +	write_sysreg(0xFFFFFFFF, pmintenclr_el1);
> +	isb();
> +}
> +
> +static void test_event_counter_config(void)
> +{
> +	int i;
> +
> +	if (!pmu.nb_implemented_counters) {
> +		report_skip("No event counter, skip ...");
> +		return;
> +	}
> +
> +	pmu_reset();
> +
> +	/*
> +	 * Test setting through PMESELR/PMXEVTYPER and PMEVTYPERn read,
> +	 * select counter 0
> +	 */
> +	write_sysreg(1, PMSELR_EL0);
> +	/* program this counter to count unsupported event */
> +	write_sysreg(0xEA, PMXEVTYPER_EL0);
> +	write_sysreg(0xdeadbeef, PMXEVCNTR_EL0);
> +	report((read_regn(pmevtyper, 1) & 0xFFF) == 0xEA,
> +		"PMESELR/PMXEVTYPER/PMEVTYPERn");
> +	report((read_regn(pmevcntr, 1) == 0xdeadbeef),
> +		"PMESELR/PMXEVCNTR/PMEVCNTRn");
> +
> +	/* try to configure an unsupported event within the range [0x0, 0x3F] */
> +	for (i = 0; i <= 0x3F; i++) {
> +		if (!is_event_supported(i, false))
> +			break;
> +	}
> +	if (i > 0x3F) {
> +		report_skip("pmevtyper: all events within [0x0, 0x3F] are supported");
> +		return;
> +	}
> +
> +	/* select counter 0 */
> +	write_sysreg(0, PMSELR_EL0);
> +	/* program this counter to count unsupported event */
> +	write_sysreg(i, PMXEVCNTR_EL0);
> +	/* read the counter value */
> +	read_sysreg(PMXEVCNTR_EL0);
> +	report(read_sysreg(PMXEVCNTR_EL0) == i,
> +		"read of a counter programmed with unsupported event");
> +
> +}
> +
> +static bool satisfy_prerequisites(uint32_t *events, unsigned int nb_events)
> +{
> +	int i;
> +
> +	if (pmu.nb_implemented_counters < nb_events) {
> +		report_skip("Skip test as number of counters is too small (%d)",
> +			    pmu.nb_implemented_counters);
> +		return false;
> +	}
> +
> +	for (i = 0; i < nb_events; i++) {
> +		if (!is_event_supported(events[i], false)) {
> +			report_skip("Skip test as event %d is not supported",
> +				    events[i]);
> +			return false;
> +		}
> +	}
> +	return true;
> +}
> +
> +static void test_basic_event_count(void)
> +{
> +	uint32_t implemented_counter_mask, non_implemented_counter_mask;
> +	uint32_t counter_mask;
> +	uint32_t events[] = {
> +		0x11,	/* CPU_CYCLES */
> +		0x8,	/* INST_RETIRED */
> +	};
> +
> +	if (!satisfy_prerequisites(events, ARRAY_SIZE(events)))
> +		return;
> +
> +	implemented_counter_mask = BIT(pmu.nb_implemented_counters) - 1;
> +	non_implemented_counter_mask = ~(BIT(31) | implemented_counter_mask);
> +	counter_mask = implemented_counter_mask | non_implemented_counter_mask;
> +
> +	write_regn(pmevtyper, 0, events[0] | PMEVTYPER_EXCLUDE_EL0);
> +	write_regn(pmevtyper, 1, events[1] | PMEVTYPER_EXCLUDE_EL0);
> +
> +	/* disable all counters */
> +	write_sysreg_s(0xFFFFFFFF, PMCNTENCLR_EL0);
> +	report(!read_sysreg_s(PMCNTENCLR_EL0) && !read_sysreg_s(PMCNTENSET_EL0),
> +		"pmcntenclr: disable all counters");
> +
> +	/*
> +	 * clear cycle and all event counters and allow counter enablement
> +	 * through PMCNTENSET. LC is RES1.
> +	 */
> +	set_pmcr(pmu.pmcr_ro | PMU_PMCR_LC | PMU_PMCR_C | PMU_PMCR_P);
> +	isb();
> +	report(get_pmcr() == (pmu.pmcr_ro | PMU_PMCR_LC), "pmcr: reset counters");
> +
> +	/* Preset counter #0 to 0xFFFFFFF0 to trigger an overflow interrupt */
> +	write_regn(pmevcntr, 0, 0xFFFFFFF0);
> +	report(read_regn(pmevcntr, 0) == 0xFFFFFFF0,
> +		"counter #0 preset to 0xFFFFFFF0");
> +	report(!read_regn(pmevcntr, 1), "counter #1 is 0");
> +
> +	/*
> +	 * Enable all implemented counters and also attempt to enable
> +	 * not supported counters. Counting still is disabled by !PMCR.E
> +	 */
> +	write_sysreg_s(counter_mask, PMCNTENSET_EL0);
> +
> +	/* check only those implemented are enabled */
> +	report((read_sysreg_s(PMCNTENSET_EL0) == read_sysreg_s(PMCNTENCLR_EL0)) &&
> +		(read_sysreg_s(PMCNTENSET_EL0) == implemented_counter_mask),
> +		"pmcntenset: enabled implemented_counters");
> +
> +	/* Disable all counters but counters #0 and #1 */
> +	write_sysreg_s(~0x3, PMCNTENCLR_EL0);
> +	report((read_sysreg_s(PMCNTENSET_EL0) == read_sysreg_s(PMCNTENCLR_EL0)) &&
> +		(read_sysreg_s(PMCNTENSET_EL0) == 0x3),
> +		"pmcntenset: just enabled #0 and #1");
> +
> +	/* clear overflow register */
> +	write_sysreg(0xFFFFFFFF, pmovsclr_el0);
> +	report(!read_sysreg(pmovsclr_el0), "check overflow reg is 0");
> +
> +	/* disable overflow interrupts on all counters*/
> +	write_sysreg(0xFFFFFFFF, pmintenclr_el1);
> +	report(!read_sysreg(pmintenclr_el1),
> +		"pmintenclr_el1=0, all interrupts disabled");
> +
> +	/* enable overflow interrupts on all event counters */
> +	write_sysreg(implemented_counter_mask | non_implemented_counter_mask,
> +		     pmintenset_el1);
> +	report(read_sysreg(pmintenset_el1) == implemented_counter_mask,
> +		"overflow interrupts enabled on all implemented counters");
> +
> +	/* Set PMCR.E, execute asm code and unset PMCR.E */
> +	precise_instrs_loop(20, pmu.pmcr_ro | PMU_PMCR_E);
> +
> +	report_info("counter #0 is 0x%lx (CPU_CYCLES)",
> +		    read_regn(pmevcntr, 0));
> +	report_info("counter #1 is 0x%lx (INST_RETIRED)",
> +		    read_regn(pmevcntr, 1));
> +
> +	report_info("overflow reg = 0x%lx", read_sysreg(pmovsclr_el0));
> +	report(read_sysreg(pmovsclr_el0) & 0x1,
> +		"check overflow happened on #0 only");
> +}
> +
> +static void test_mem_access(void)
> +{
> +	void *addr = malloc(PAGE_SIZE);
> +	uint32_t events[] = {
> +		0x13,   /* MEM_ACCESS */
> +		0x13,   /* MEM_ACCESS */
> +	};
> +
> +	if (!satisfy_prerequisites(events, ARRAY_SIZE(events)))
> +		return;
> +
> +	pmu_reset();
> +
> +	write_regn(pmevtyper, 0, events[0] | PMEVTYPER_EXCLUDE_EL0);
> +	write_regn(pmevtyper, 1, events[1] | PMEVTYPER_EXCLUDE_EL0);
> +	write_sysreg_s(0x3, PMCNTENSET_EL0);
> +	isb();
> +	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E);
> +	report_info("counter #0 is %ld (MEM_ACCESS)", read_regn(pmevcntr, 0));
> +	report_info("counter #1 is %ld (MEM_ACCESS)", read_regn(pmevcntr, 1));
> +	/* We may measure more than 20 mem access depending on the core */
> +	report((read_regn(pmevcntr, 0) == read_regn(pmevcntr, 1)) &&
> +	       (read_regn(pmevcntr, 0) >= 20) && !read_sysreg(pmovsclr_el0),
> +	       "Ran 20 mem accesses");
> +
> +	pmu_reset();
> +
> +	write_regn(pmevcntr, 0, 0xFFFFFFFA);
> +	write_regn(pmevcntr, 1, 0xFFFFFFF0);
> +	write_sysreg_s(0x3, PMCNTENSET_EL0);
> +	isb();
> +	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E);
> +	report(read_sysreg(pmovsclr_el0) == 0x3,
> +	       "Ran 20 mem accesses with expected overflows on both counters");
> +	report_info("cnt#0 = %ld cnt#1=%ld overflow=0x%lx",
> +			read_regn(pmevcntr, 0), read_regn(pmevcntr, 1),
> +			read_sysreg(pmovsclr_el0));
> +}
> +
>  #endif
>  
>  /*
> @@ -388,6 +648,15 @@ int main(int argc, char *argv[])
>  	} else if (strcmp(argv[1], "event-introspection") == 0) {
>  		report_prefix_push(argv[1]);
>  		test_event_introspection();
> +	} else if (strcmp(argv[1], "event-counter-config") == 0) {
> +		report_prefix_push(argv[1]);
> +		test_event_counter_config();
> +	} else if (strcmp(argv[1], "basic-event-count") == 0) {
> +		report_prefix_push(argv[1]);
> +		test_basic_event_count();
> +	} else if (strcmp(argv[1], "mem-access") == 0) {
> +		report_prefix_push(argv[1]);
> +		test_mem_access();
>  	} else {
>  		report_abort("Unknown sub-test '%s'", argv[1]);
>  	}
> diff --git a/arm/unittests.cfg b/arm/unittests.cfg
> index 4433ef3..7a59403 100644
> --- a/arm/unittests.cfg
> +++ b/arm/unittests.cfg
> @@ -72,6 +72,24 @@ groups = pmu
>  arch = arm64
>  extra_params = -append 'event-introspection'
>  
> +[pmu-event-counter-config]
> +file = pmu.flat
> +groups = pmu
> +arch = arm64
> +extra_params = -append 'event-counter-config'
> +
> +[pmu-basic-event-count]
> +file = pmu.flat
> +groups = pmu
> +arch = arm64
> +extra_params = -append 'basic-event-count'
> +
> +[pmu-mem-access]
> +file = pmu.flat
> +groups = pmu
> +arch = arm64
> +extra_params = -append 'mem-access'
> +
>  # Test PMU support (TCG) with -icount IPC=1
>  #[pmu-tcg-icount-1]
>  #file = pmu.flat

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [kvm-unit-tests PATCH v2 5/9] arm: pmu: Basic event counter Tests

[Andrew Jones]

On Thu, Jan 30, 2020 at 12:25:06PM +0100, Eric Auger wrote:
> Adds the following tests:
> - event-counter-config: test event counter configuration
> - basic-event-count:
>   - programs counters #0 and #1 to count 2 required events
>   (resp. CPU_CYCLES and INST_RETIRED). Counter #0 is preset
>   to a value close enough to the 32b
>   overflow limit so that we check the overflow bit is set
>   after the execution of the asm loop.
> - mem-access: counts MEM_ACCESS event on counters #0 and #1
>   with and without 32-bit overflow.
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> 
> ---
> 
> v1 -> v2:
> - fix PMCNTENSET_EL0 and PMCNTENCLR_EL0 op0
> - print PMEVTYPER SH
> - properly clobber used regs and add "cc"
> - simplify mem_access_loop
> ---
>  arm/pmu.c         | 269 ++++++++++++++++++++++++++++++++++++++++++++++
>  arm/unittests.cfg |  18 ++++
>  2 files changed, 287 insertions(+)
> 
> diff --git a/arm/pmu.c b/arm/pmu.c
> index 4a26a76..1b0101f 100644
> --- a/arm/pmu.c
> +++ b/arm/pmu.c
> @@ -18,9 +18,15 @@
>  #include "asm/barrier.h"
>  #include "asm/sysreg.h"
>  #include "asm/processor.h"
> +#include <bitops.h>
> +#include <asm/gic.h>
>  
>  #define PMU_PMCR_E         (1 << 0)
> +#define PMU_PMCR_P         (1 << 1)
>  #define PMU_PMCR_C         (1 << 2)
> +#define PMU_PMCR_D         (1 << 3)
> +#define PMU_PMCR_X         (1 << 4)
> +#define PMU_PMCR_DP        (1 << 5)
>  #define PMU_PMCR_LC        (1 << 6)
>  #define PMU_PMCR_N_SHIFT   11
>  #define PMU_PMCR_N_MASK    0x1f
> @@ -104,6 +110,9 @@ static inline void precise_instrs_loop(int loop, uint32_t pmcr)
>  
>  /* event counter tests only implemented for aarch64 */
>  static void test_event_introspection(void) {}
> +static void test_event_counter_config(void) {}
> +static void test_basic_event_count(void) {}
> +static void test_mem_access(void) {}
>  
>  #elif defined(__aarch64__)
>  #define ID_AA64DFR0_PERFMON_SHIFT 8
> @@ -145,6 +154,33 @@ static inline void precise_instrs_loop(int loop, uint32_t pmcr)
>  }
>  
>  #define PMCEID1_EL0 sys_reg(3, 3, 9, 12, 7)
> +#define PMCNTENSET_EL0 sys_reg(3, 3, 9, 12, 1)
> +#define PMCNTENCLR_EL0 sys_reg(3, 3, 9, 12, 2)
> +
> +#define PMEVTYPER_EXCLUDE_EL1 (1 << 31)
> +#define PMEVTYPER_EXCLUDE_EL0 (1 << 30)
> +
> +#define regn_el0(__reg, __n) __reg ## __n  ## _el0
> +#define write_regn(__reg, __n, __val) \
> +	write_sysreg((__val), __reg ## __n ## _el0)
> +
> +#define read_regn(__reg, __n) \
> +	read_sysreg(__reg ## __n ## _el0)

You can delete regn_el0() since you don't use it anyway.

The el0 should probably be in the macro names,

write_regn_el0()
read_regn_el0()

Also they could go in lib/arm64/asm/sysreg.h

> +
> +#define print_pmevtyper(__s, __n) do { \
> +	uint32_t val; \
> +	val = read_regn(pmevtyper, __n);\
> +	report_info("%s pmevtyper%d=0x%x, eventcount=0x%x (p=%ld, u=%ld nsk=%ld, nsu=%ld, nsh=%ld m=%ld, mt=%ld, sh=%ld)", \
> +			(__s), (__n), val, val & 0xFFFF,	\
> +			(BIT_MASK(31) & val) >> 31,		\
> +			(BIT_MASK(30) & val) >> 30,		\
> +			(BIT_MASK(29) & val) >> 29,		\
> +			(BIT_MASK(28) & val) >> 28,		\
> +			(BIT_MASK(27) & val) >> 27,		\
> +			(BIT_MASK(26) & val) >> 26,		\
> +			(BIT_MASK(25) & val) >> 25);		\
> +			(BIT_MASK(24) & val) >> 24);		\
> +	} while (0)
>  
>  static bool is_event_supported(uint32_t n, bool warn)
>  {
> @@ -198,6 +234,230 @@ static void test_event_introspection(void)
>  	report(required_events, "Check required events are implemented");
>  }
>  
> +/*
> + * 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. Loads @loop times the data at @address into x9.
> + */
> +static void mem_access_loop(void *addr, int loop, uint32_t pmcr)
> +{
> +asm volatile(
> +	"       msr     pmcr_el0, %[pmcr]\n"
> +	"       isb\n"
> +	"       mov     x10, %[loop]\n"
> +	"1:     sub     x10, x10, #1\n"
> +	"       ldr x9, [%[addr]]\n"

indent is wrong for line above

> +	"       cmp     x10, #0x0\n"
> +	"       b.gt    1b\n"
> +	"       msr     pmcr_el0, xzr\n"
> +	"       isb\n"
> +	:
> +	: [addr] "r" (addr), [pmcr] "r" (pmcr), [loop] "r" (loop)
> +	: "x9", "x10", "cc");
> +}
> +
> +static void pmu_reset(void)
> +{
> +	/* reset all counters, counting disabled at PMCR level*/
> +	set_pmcr(pmu.pmcr_ro | PMU_PMCR_LC | PMU_PMCR_C | PMU_PMCR_P);
> +	/* Disable all counters */
> +	write_sysreg_s(0xFFFFFFFF, PMCNTENCLR_EL0);
> +	/* clear overflow reg */
> +	write_sysreg(0xFFFFFFFF, pmovsclr_el0);
> +	/* disable overflow interrupts on all counters */
> +	write_sysreg(0xFFFFFFFF, pmintenclr_el1);
> +	isb();
> +}
> +
> +static void test_event_counter_config(void)
> +{
> +	int i;
> +
> +	if (!pmu.nb_implemented_counters) {
> +		report_skip("No event counter, skip ...");
> +		return;
> +	}
> +
> +	pmu_reset();
> +
> +	/*
> +	 * Test setting through PMESELR/PMXEVTYPER and PMEVTYPERn read,
> +	 * select counter 0
> +	 */
> +	write_sysreg(1, PMSELR_EL0);
> +	/* program this counter to count unsupported event */
> +	write_sysreg(0xEA, PMXEVTYPER_EL0);
> +	write_sysreg(0xdeadbeef, PMXEVCNTR_EL0);
> +	report((read_regn(pmevtyper, 1) & 0xFFF) == 0xEA,
> +		"PMESELR/PMXEVTYPER/PMEVTYPERn");
> +	report((read_regn(pmevcntr, 1) == 0xdeadbeef),
> +		"PMESELR/PMXEVCNTR/PMEVCNTRn");
> +
> +	/* try to configure an unsupported event within the range [0x0, 0x3F] */
> +	for (i = 0; i <= 0x3F; i++) {
> +		if (!is_event_supported(i, false))
> +			break;
> +	}
> +	if (i > 0x3F) {
> +		report_skip("pmevtyper: all events within [0x0, 0x3F] are supported");
> +		return;
> +	}
> +
> +	/* select counter 0 */
> +	write_sysreg(0, PMSELR_EL0);
> +	/* program this counter to count unsupported event */
> +	write_sysreg(i, PMXEVCNTR_EL0);
> +	/* read the counter value */
> +	read_sysreg(PMXEVCNTR_EL0);
> +	report(read_sysreg(PMXEVCNTR_EL0) == i,
> +		"read of a counter programmed with unsupported event");
> +
> +}
> +
> +static bool satisfy_prerequisites(uint32_t *events, unsigned int nb_events)
> +{
> +	int i;
> +
> +	if (pmu.nb_implemented_counters < nb_events) {
> +		report_skip("Skip test as number of counters is too small (%d)",
> +			    pmu.nb_implemented_counters);
> +		return false;
> +	}
> +
> +	for (i = 0; i < nb_events; i++) {
> +		if (!is_event_supported(events[i], false)) {
> +			report_skip("Skip test as event %d is not supported",
> +				    events[i]);
> +			return false;
> +		}
> +	}
> +	return true;
> +}
> +
> +static void test_basic_event_count(void)
> +{
> +	uint32_t implemented_counter_mask, non_implemented_counter_mask;
> +	uint32_t counter_mask;
> +	uint32_t events[] = {
> +		0x11,	/* CPU_CYCLES */
> +		0x8,	/* INST_RETIRED */
> +	};

#define CPU_CYCLES 0x11
#define INST_RETIRED 0x8

then no need for the comments and ...

> +
> +	if (!satisfy_prerequisites(events, ARRAY_SIZE(events)))
> +		return;
> +
> +	implemented_counter_mask = BIT(pmu.nb_implemented_counters) - 1;
> +	non_implemented_counter_mask = ~(BIT(31) | implemented_counter_mask);
> +	counter_mask = implemented_counter_mask | non_implemented_counter_mask;
> +
> +	write_regn(pmevtyper, 0, events[0] | PMEVTYPER_EXCLUDE_EL0);
> +	write_regn(pmevtyper, 1, events[1] | PMEVTYPER_EXCLUDE_EL0);

... here we can see what we're doing more clearing using the defines
instead of array members.

> +
> +	/* disable all counters */
> +	write_sysreg_s(0xFFFFFFFF, PMCNTENCLR_EL0);
> +	report(!read_sysreg_s(PMCNTENCLR_EL0) && !read_sysreg_s(PMCNTENSET_EL0),
> +		"pmcntenclr: disable all counters");
> +
> +	/*
> +	 * clear cycle and all event counters and allow counter enablement
> +	 * through PMCNTENSET. LC is RES1.
> +	 */
> +	set_pmcr(pmu.pmcr_ro | PMU_PMCR_LC | PMU_PMCR_C | PMU_PMCR_P);
> +	isb();
> +	report(get_pmcr() == (pmu.pmcr_ro | PMU_PMCR_LC), "pmcr: reset counters");
> +
> +	/* Preset counter #0 to 0xFFFFFFF0 to trigger an overflow interrupt */
> +	write_regn(pmevcntr, 0, 0xFFFFFFF0);
> +	report(read_regn(pmevcntr, 0) == 0xFFFFFFF0,
> +		"counter #0 preset to 0xFFFFFFF0");
> +	report(!read_regn(pmevcntr, 1), "counter #1 is 0");
> +
> +	/*
> +	 * Enable all implemented counters and also attempt to enable
> +	 * not supported counters. Counting still is disabled by !PMCR.E
> +	 */
> +	write_sysreg_s(counter_mask, PMCNTENSET_EL0);
> +
> +	/* check only those implemented are enabled */
> +	report((read_sysreg_s(PMCNTENSET_EL0) == read_sysreg_s(PMCNTENCLR_EL0)) &&
> +		(read_sysreg_s(PMCNTENSET_EL0) == implemented_counter_mask),
> +		"pmcntenset: enabled implemented_counters");
> +
> +	/* Disable all counters but counters #0 and #1 */
> +	write_sysreg_s(~0x3, PMCNTENCLR_EL0);

First time you've used ~ to define a mask. I prefer this to all the
0xFFF..'s, but I prefer consistency and defines even more.

> +	report((read_sysreg_s(PMCNTENSET_EL0) == read_sysreg_s(PMCNTENCLR_EL0)) &&
> +		(read_sysreg_s(PMCNTENSET_EL0) == 0x3),
> +		"pmcntenset: just enabled #0 and #1");
> +
> +	/* clear overflow register */
> +	write_sysreg(0xFFFFFFFF, pmovsclr_el0);
> +	report(!read_sysreg(pmovsclr_el0), "check overflow reg is 0");
> +
> +	/* disable overflow interrupts on all counters*/
> +	write_sysreg(0xFFFFFFFF, pmintenclr_el1);
> +	report(!read_sysreg(pmintenclr_el1),
> +		"pmintenclr_el1=0, all interrupts disabled");
> +
> +	/* enable overflow interrupts on all event counters */
> +	write_sysreg(implemented_counter_mask | non_implemented_counter_mask,
> +		     pmintenset_el1);
> +	report(read_sysreg(pmintenset_el1) == implemented_counter_mask,
> +		"overflow interrupts enabled on all implemented counters");
> +
> +	/* Set PMCR.E, execute asm code and unset PMCR.E */
> +	precise_instrs_loop(20, pmu.pmcr_ro | PMU_PMCR_E);
> +
> +	report_info("counter #0 is 0x%lx (CPU_CYCLES)",
> +		    read_regn(pmevcntr, 0));
> +	report_info("counter #1 is 0x%lx (INST_RETIRED)",
> +		    read_regn(pmevcntr, 1));
> +
> +	report_info("overflow reg = 0x%lx", read_sysreg(pmovsclr_el0));
> +	report(read_sysreg(pmovsclr_el0) & 0x1,
> +		"check overflow happened on #0 only");
> +}
> +
> +static void test_mem_access(void)
> +{
> +	void *addr = malloc(PAGE_SIZE);
> +	uint32_t events[] = {
> +		0x13,   /* MEM_ACCESS */
> +		0x13,   /* MEM_ACCESS */
> +	};

#define MEM_ACCESS 0x13

> +
> +	if (!satisfy_prerequisites(events, ARRAY_SIZE(events)))

No need to check the same event twice. Just call satisfy_prerequisites
with ((uint32_t []){ MEM_ACCESS }, 1)

> +		return;
> +
> +	pmu_reset();
> +
> +	write_regn(pmevtyper, 0, events[0] | PMEVTYPER_EXCLUDE_EL0);
> +	write_regn(pmevtyper, 1, events[1] | PMEVTYPER_EXCLUDE_EL0);

Same comment on using defines rather than array members.

> +	write_sysreg_s(0x3, PMCNTENSET_EL0);
> +	isb();
> +	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E);
> +	report_info("counter #0 is %ld (MEM_ACCESS)", read_regn(pmevcntr, 0));
> +	report_info("counter #1 is %ld (MEM_ACCESS)", read_regn(pmevcntr, 1));
> +	/* We may measure more than 20 mem access depending on the core */
> +	report((read_regn(pmevcntr, 0) == read_regn(pmevcntr, 1)) &&
> +	       (read_regn(pmevcntr, 0) >= 20) && !read_sysreg(pmovsclr_el0),
> +	       "Ran 20 mem accesses");
> +
> +	pmu_reset();
> +
> +	write_regn(pmevcntr, 0, 0xFFFFFFFA);
> +	write_regn(pmevcntr, 1, 0xFFFFFFF0);
> +	write_sysreg_s(0x3, PMCNTENSET_EL0);
> +	isb();
> +	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E);
> +	report(read_sysreg(pmovsclr_el0) == 0x3,
> +	       "Ran 20 mem accesses with expected overflows on both counters");
> +	report_info("cnt#0 = %ld cnt#1=%ld overflow=0x%lx",
> +			read_regn(pmevcntr, 0), read_regn(pmevcntr, 1),
> +			read_sysreg(pmovsclr_el0));
> +}
> +
>  #endif
>  
>  /*
> @@ -388,6 +648,15 @@ int main(int argc, char *argv[])
>  	} else if (strcmp(argv[1], "event-introspection") == 0) {
>  		report_prefix_push(argv[1]);
>  		test_event_introspection();
> +	} else if (strcmp(argv[1], "event-counter-config") == 0) {
> +		report_prefix_push(argv[1]);
> +		test_event_counter_config();
> +	} else if (strcmp(argv[1], "basic-event-count") == 0) {
> +		report_prefix_push(argv[1]);
> +		test_basic_event_count();
> +	} else if (strcmp(argv[1], "mem-access") == 0) {
> +		report_prefix_push(argv[1]);
> +		test_mem_access();

missing the pops. I know they're not necessary since we finish the tests
afterwards, but the universe must stay balanced. Where there's a yin, we
should put a yang.

>  	} else {
>  		report_abort("Unknown sub-test '%s'", argv[1]);
>  	}
> diff --git a/arm/unittests.cfg b/arm/unittests.cfg
> index 4433ef3..7a59403 100644
> --- a/arm/unittests.cfg
> +++ b/arm/unittests.cfg
> @@ -72,6 +72,24 @@ groups = pmu
>  arch = arm64
>  extra_params = -append 'event-introspection'
>  
> +[pmu-event-counter-config]
> +file = pmu.flat
> +groups = pmu
> +arch = arm64
> +extra_params = -append 'event-counter-config'
> +
> +[pmu-basic-event-count]
> +file = pmu.flat
> +groups = pmu
> +arch = arm64
> +extra_params = -append 'basic-event-count'
> +
> +[pmu-mem-access]
> +file = pmu.flat
> +groups = pmu
> +arch = arm64
> +extra_params = -append 'mem-access'
> +
>  # Test PMU support (TCG) with -icount IPC=1
>  #[pmu-tcg-icount-1]
>  #file = pmu.flat
> -- 
> 2.20.1
> 
>

I only skimmed this looking for style/framework issues. I'd prefer more
defines in general to all the hex, but I won't insist on that.

Thanks,
drew

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [kvm-unit-tests PATCH v2 5/9] arm: pmu: Basic event counter Tests

[Auger Eric]

Hi Drew,

On 3/5/20 10:33 AM, Andrew Jones wrote:
> On Thu, Jan 30, 2020 at 12:25:06PM +0100, Eric Auger wrote:
>> Adds the following tests:
>> - event-counter-config: test event counter configuration
>> - basic-event-count:
>>   - programs counters #0 and #1 to count 2 required events
>>   (resp. CPU_CYCLES and INST_RETIRED). Counter #0 is preset
>>   to a value close enough to the 32b
>>   overflow limit so that we check the overflow bit is set
>>   after the execution of the asm loop.
>> - mem-access: counts MEM_ACCESS event on counters #0 and #1
>>   with and without 32-bit overflow.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>
>> ---
>>
>> v1 -> v2:
>> - fix PMCNTENSET_EL0 and PMCNTENCLR_EL0 op0
>> - print PMEVTYPER SH
>> - properly clobber used regs and add "cc"
>> - simplify mem_access_loop
>> ---
>>  arm/pmu.c         | 269 ++++++++++++++++++++++++++++++++++++++++++++++
>>  arm/unittests.cfg |  18 ++++
>>  2 files changed, 287 insertions(+)
>>
>> diff --git a/arm/pmu.c b/arm/pmu.c
>> index 4a26a76..1b0101f 100644
>> --- a/arm/pmu.c
>> +++ b/arm/pmu.c
>> @@ -18,9 +18,15 @@
>>  #include "asm/barrier.h"
>>  #include "asm/sysreg.h"
>>  #include "asm/processor.h"
>> +#include <bitops.h>
>> +#include <asm/gic.h>
>>  
>>  #define PMU_PMCR_E         (1 << 0)
>> +#define PMU_PMCR_P         (1 << 1)
>>  #define PMU_PMCR_C         (1 << 2)
>> +#define PMU_PMCR_D         (1 << 3)
>> +#define PMU_PMCR_X         (1 << 4)
>> +#define PMU_PMCR_DP        (1 << 5)
>>  #define PMU_PMCR_LC        (1 << 6)
>>  #define PMU_PMCR_N_SHIFT   11
>>  #define PMU_PMCR_N_MASK    0x1f
>> @@ -104,6 +110,9 @@ static inline void precise_instrs_loop(int loop, uint32_t pmcr)
>>  
>>  /* event counter tests only implemented for aarch64 */
>>  static void test_event_introspection(void) {}
>> +static void test_event_counter_config(void) {}
>> +static void test_basic_event_count(void) {}
>> +static void test_mem_access(void) {}
>>  
>>  #elif defined(__aarch64__)
>>  #define ID_AA64DFR0_PERFMON_SHIFT 8
>> @@ -145,6 +154,33 @@ static inline void precise_instrs_loop(int loop, uint32_t pmcr)
>>  }
>>  
>>  #define PMCEID1_EL0 sys_reg(3, 3, 9, 12, 7)
>> +#define PMCNTENSET_EL0 sys_reg(3, 3, 9, 12, 1)
>> +#define PMCNTENCLR_EL0 sys_reg(3, 3, 9, 12, 2)
>> +
>> +#define PMEVTYPER_EXCLUDE_EL1 (1 << 31)
>> +#define PMEVTYPER_EXCLUDE_EL0 (1 << 30)
>> +
>> +#define regn_el0(__reg, __n) __reg ## __n  ## _el0
>> +#define write_regn(__reg, __n, __val) \
>> +	write_sysreg((__val), __reg ## __n ## _el0)
>> +
>> +#define read_regn(__reg, __n) \
>> +	read_sysreg(__reg ## __n ## _el0)
> 
> You can delete regn_el0() since you don't use it anyway.
> 
> The el0 should probably be in the macro names,
> 
> write_regn_el0()
> read_regn_el0()
> 
> Also they could go in lib/arm64/asm/sysreg.h
> 
>> +
>> +#define print_pmevtyper(__s, __n) do { \
>> +	uint32_t val; \
>> +	val = read_regn(pmevtyper, __n);\
>> +	report_info("%s pmevtyper%d=0x%x, eventcount=0x%x (p=%ld, u=%ld nsk=%ld, nsu=%ld, nsh=%ld m=%ld, mt=%ld, sh=%ld)", \
>> +			(__s), (__n), val, val & 0xFFFF,	\
>> +			(BIT_MASK(31) & val) >> 31,		\
>> +			(BIT_MASK(30) & val) >> 30,		\
>> +			(BIT_MASK(29) & val) >> 29,		\
>> +			(BIT_MASK(28) & val) >> 28,		\
>> +			(BIT_MASK(27) & val) >> 27,		\
>> +			(BIT_MASK(26) & val) >> 26,		\
>> +			(BIT_MASK(25) & val) >> 25);		\
>> +			(BIT_MASK(24) & val) >> 24);		\
>> +	} while (0)
>>  
>>  static bool is_event_supported(uint32_t n, bool warn)
>>  {
>> @@ -198,6 +234,230 @@ static void test_event_introspection(void)
>>  	report(required_events, "Check required events are implemented");
>>  }
>>  
>> +/*
>> + * 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. Loads @loop times the data at @address into x9.
>> + */
>> +static void mem_access_loop(void *addr, int loop, uint32_t pmcr)
>> +{
>> +asm volatile(
>> +	"       msr     pmcr_el0, %[pmcr]\n"
>> +	"       isb\n"
>> +	"       mov     x10, %[loop]\n"
>> +	"1:     sub     x10, x10, #1\n"
>> +	"       ldr x9, [%[addr]]\n"
> 
> indent is wrong for line above
> 
>> +	"       cmp     x10, #0x0\n"
>> +	"       b.gt    1b\n"
>> +	"       msr     pmcr_el0, xzr\n"
>> +	"       isb\n"
>> +	:
>> +	: [addr] "r" (addr), [pmcr] "r" (pmcr), [loop] "r" (loop)
>> +	: "x9", "x10", "cc");
>> +}
>> +
>> +static void pmu_reset(void)
>> +{
>> +	/* reset all counters, counting disabled at PMCR level*/
>> +	set_pmcr(pmu.pmcr_ro | PMU_PMCR_LC | PMU_PMCR_C | PMU_PMCR_P);
>> +	/* Disable all counters */
>> +	write_sysreg_s(0xFFFFFFFF, PMCNTENCLR_EL0);
>> +	/* clear overflow reg */
>> +	write_sysreg(0xFFFFFFFF, pmovsclr_el0);
>> +	/* disable overflow interrupts on all counters */
>> +	write_sysreg(0xFFFFFFFF, pmintenclr_el1);
>> +	isb();
>> +}
>> +
>> +static void test_event_counter_config(void)
>> +{
>> +	int i;
>> +
>> +	if (!pmu.nb_implemented_counters) {
>> +		report_skip("No event counter, skip ...");
>> +		return;
>> +	}
>> +
>> +	pmu_reset();
>> +
>> +	/*
>> +	 * Test setting through PMESELR/PMXEVTYPER and PMEVTYPERn read,
>> +	 * select counter 0
>> +	 */
>> +	write_sysreg(1, PMSELR_EL0);
>> +	/* program this counter to count unsupported event */
>> +	write_sysreg(0xEA, PMXEVTYPER_EL0);
>> +	write_sysreg(0xdeadbeef, PMXEVCNTR_EL0);
>> +	report((read_regn(pmevtyper, 1) & 0xFFF) == 0xEA,
>> +		"PMESELR/PMXEVTYPER/PMEVTYPERn");
>> +	report((read_regn(pmevcntr, 1) == 0xdeadbeef),
>> +		"PMESELR/PMXEVCNTR/PMEVCNTRn");
>> +
>> +	/* try to configure an unsupported event within the range [0x0, 0x3F] */
>> +	for (i = 0; i <= 0x3F; i++) {
>> +		if (!is_event_supported(i, false))
>> +			break;
>> +	}
>> +	if (i > 0x3F) {
>> +		report_skip("pmevtyper: all events within [0x0, 0x3F] are supported");
>> +		return;
>> +	}
>> +
>> +	/* select counter 0 */
>> +	write_sysreg(0, PMSELR_EL0);
>> +	/* program this counter to count unsupported event */
>> +	write_sysreg(i, PMXEVCNTR_EL0);
>> +	/* read the counter value */
>> +	read_sysreg(PMXEVCNTR_EL0);
>> +	report(read_sysreg(PMXEVCNTR_EL0) == i,
>> +		"read of a counter programmed with unsupported event");
>> +
>> +}
>> +
>> +static bool satisfy_prerequisites(uint32_t *events, unsigned int nb_events)
>> +{
>> +	int i;
>> +
>> +	if (pmu.nb_implemented_counters < nb_events) {
>> +		report_skip("Skip test as number of counters is too small (%d)",
>> +			    pmu.nb_implemented_counters);
>> +		return false;
>> +	}
>> +
>> +	for (i = 0; i < nb_events; i++) {
>> +		if (!is_event_supported(events[i], false)) {
>> +			report_skip("Skip test as event %d is not supported",
>> +				    events[i]);
>> +			return false;
>> +		}
>> +	}
>> +	return true;
>> +}
>> +
>> +static void test_basic_event_count(void)
>> +{
>> +	uint32_t implemented_counter_mask, non_implemented_counter_mask;
>> +	uint32_t counter_mask;
>> +	uint32_t events[] = {
>> +		0x11,	/* CPU_CYCLES */
>> +		0x8,	/* INST_RETIRED */
>> +	};
> 
> #define CPU_CYCLES 0x11
> #define INST_RETIRED 0x8
> 
> then no need for the comments and ...
> 
>> +
>> +	if (!satisfy_prerequisites(events, ARRAY_SIZE(events)))
>> +		return;
>> +
>> +	implemented_counter_mask = BIT(pmu.nb_implemented_counters) - 1;
>> +	non_implemented_counter_mask = ~(BIT(31) | implemented_counter_mask);
>> +	counter_mask = implemented_counter_mask | non_implemented_counter_mask;
>> +
>> +	write_regn(pmevtyper, 0, events[0] | PMEVTYPER_EXCLUDE_EL0);
>> +	write_regn(pmevtyper, 1, events[1] | PMEVTYPER_EXCLUDE_EL0);
> 
> ... here we can see what we're doing more clearing using the defines
> instead of array members.
> 
>> +
>> +	/* disable all counters */
>> +	write_sysreg_s(0xFFFFFFFF, PMCNTENCLR_EL0);
>> +	report(!read_sysreg_s(PMCNTENCLR_EL0) && !read_sysreg_s(PMCNTENSET_EL0),
>> +		"pmcntenclr: disable all counters");
>> +
>> +	/*
>> +	 * clear cycle and all event counters and allow counter enablement
>> +	 * through PMCNTENSET. LC is RES1.
>> +	 */
>> +	set_pmcr(pmu.pmcr_ro | PMU_PMCR_LC | PMU_PMCR_C | PMU_PMCR_P);
>> +	isb();
>> +	report(get_pmcr() == (pmu.pmcr_ro | PMU_PMCR_LC), "pmcr: reset counters");
>> +
>> +	/* Preset counter #0 to 0xFFFFFFF0 to trigger an overflow interrupt */
>> +	write_regn(pmevcntr, 0, 0xFFFFFFF0);
>> +	report(read_regn(pmevcntr, 0) == 0xFFFFFFF0,
>> +		"counter #0 preset to 0xFFFFFFF0");
>> +	report(!read_regn(pmevcntr, 1), "counter #1 is 0");
>> +
>> +	/*
>> +	 * Enable all implemented counters and also attempt to enable
>> +	 * not supported counters. Counting still is disabled by !PMCR.E
>> +	 */
>> +	write_sysreg_s(counter_mask, PMCNTENSET_EL0);
>> +
>> +	/* check only those implemented are enabled */
>> +	report((read_sysreg_s(PMCNTENSET_EL0) == read_sysreg_s(PMCNTENCLR_EL0)) &&
>> +		(read_sysreg_s(PMCNTENSET_EL0) == implemented_counter_mask),
>> +		"pmcntenset: enabled implemented_counters");
>> +
>> +	/* Disable all counters but counters #0 and #1 */
>> +	write_sysreg_s(~0x3, PMCNTENCLR_EL0);
> 
> First time you've used ~ to define a mask. I prefer this to all the
> 0xFFF..'s, but I prefer consistency and defines even more.
> 
>> +	report((read_sysreg_s(PMCNTENSET_EL0) == read_sysreg_s(PMCNTENCLR_EL0)) &&
>> +		(read_sysreg_s(PMCNTENSET_EL0) == 0x3),
>> +		"pmcntenset: just enabled #0 and #1");
>> +
>> +	/* clear overflow register */
>> +	write_sysreg(0xFFFFFFFF, pmovsclr_el0);
>> +	report(!read_sysreg(pmovsclr_el0), "check overflow reg is 0");
>> +
>> +	/* disable overflow interrupts on all counters*/
>> +	write_sysreg(0xFFFFFFFF, pmintenclr_el1);
>> +	report(!read_sysreg(pmintenclr_el1),
>> +		"pmintenclr_el1=0, all interrupts disabled");
>> +
>> +	/* enable overflow interrupts on all event counters */
>> +	write_sysreg(implemented_counter_mask | non_implemented_counter_mask,
>> +		     pmintenset_el1);
>> +	report(read_sysreg(pmintenset_el1) == implemented_counter_mask,
>> +		"overflow interrupts enabled on all implemented counters");
>> +
>> +	/* Set PMCR.E, execute asm code and unset PMCR.E */
>> +	precise_instrs_loop(20, pmu.pmcr_ro | PMU_PMCR_E);
>> +
>> +	report_info("counter #0 is 0x%lx (CPU_CYCLES)",
>> +		    read_regn(pmevcntr, 0));
>> +	report_info("counter #1 is 0x%lx (INST_RETIRED)",
>> +		    read_regn(pmevcntr, 1));
>> +
>> +	report_info("overflow reg = 0x%lx", read_sysreg(pmovsclr_el0));
>> +	report(read_sysreg(pmovsclr_el0) & 0x1,
>> +		"check overflow happened on #0 only");
>> +}
>> +
>> +static void test_mem_access(void)
>> +{
>> +	void *addr = malloc(PAGE_SIZE);
>> +	uint32_t events[] = {
>> +		0x13,   /* MEM_ACCESS */
>> +		0x13,   /* MEM_ACCESS */
>> +	};
> 
> #define MEM_ACCESS 0x13
> 
>> +
>> +	if (!satisfy_prerequisites(events, ARRAY_SIZE(events)))
> 
> No need to check the same event twice. Just call satisfy_prerequisites
> with ((uint32_t []){ MEM_ACCESS }, 1)
satisfy_prerequisites also test the number of implemented counters match
the ARRAY_SIZE.

In Practice most of my tests use 2 counters. I could skip the tests if
we don't have 2 counters.

But well ...

Eric
> 
>> +		return;
>> +
>> +	pmu_reset();
>> +
>> +	write_regn(pmevtyper, 0, events[0] | PMEVTYPER_EXCLUDE_EL0);
>> +	write_regn(pmevtyper, 1, events[1] | PMEVTYPER_EXCLUDE_EL0);
> 
> Same comment on using defines rather than array members.
> 
>> +	write_sysreg_s(0x3, PMCNTENSET_EL0);
>> +	isb();
>> +	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E);
>> +	report_info("counter #0 is %ld (MEM_ACCESS)", read_regn(pmevcntr, 0));
>> +	report_info("counter #1 is %ld (MEM_ACCESS)", read_regn(pmevcntr, 1));
>> +	/* We may measure more than 20 mem access depending on the core */
>> +	report((read_regn(pmevcntr, 0) == read_regn(pmevcntr, 1)) &&
>> +	       (read_regn(pmevcntr, 0) >= 20) && !read_sysreg(pmovsclr_el0),
>> +	       "Ran 20 mem accesses");
>> +
>> +	pmu_reset();
>> +
>> +	write_regn(pmevcntr, 0, 0xFFFFFFFA);
>> +	write_regn(pmevcntr, 1, 0xFFFFFFF0);
>> +	write_sysreg_s(0x3, PMCNTENSET_EL0);
>> +	isb();
>> +	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E);
>> +	report(read_sysreg(pmovsclr_el0) == 0x3,
>> +	       "Ran 20 mem accesses with expected overflows on both counters");
>> +	report_info("cnt#0 = %ld cnt#1=%ld overflow=0x%lx",
>> +			read_regn(pmevcntr, 0), read_regn(pmevcntr, 1),
>> +			read_sysreg(pmovsclr_el0));
>> +}
>> +
>>  #endif
>>  
>>  /*
>> @@ -388,6 +648,15 @@ int main(int argc, char *argv[])
>>  	} else if (strcmp(argv[1], "event-introspection") == 0) {
>>  		report_prefix_push(argv[1]);
>>  		test_event_introspection();
>> +	} else if (strcmp(argv[1], "event-counter-config") == 0) {
>> +		report_prefix_push(argv[1]);
>> +		test_event_counter_config();
>> +	} else if (strcmp(argv[1], "basic-event-count") == 0) {
>> +		report_prefix_push(argv[1]);
>> +		test_basic_event_count();
>> +	} else if (strcmp(argv[1], "mem-access") == 0) {
>> +		report_prefix_push(argv[1]);
>> +		test_mem_access();
> 
> missing the pops. I know they're not necessary since we finish the tests
> afterwards, but the universe must stay balanced. Where there's a yin, we
> should put a yang.
> 
>>  	} else {
>>  		report_abort("Unknown sub-test '%s'", argv[1]);
>>  	}
>> diff --git a/arm/unittests.cfg b/arm/unittests.cfg
>> index 4433ef3..7a59403 100644
>> --- a/arm/unittests.cfg
>> +++ b/arm/unittests.cfg
>> @@ -72,6 +72,24 @@ groups = pmu
>>  arch = arm64
>>  extra_params = -append 'event-introspection'
>>  
>> +[pmu-event-counter-config]
>> +file = pmu.flat
>> +groups = pmu
>> +arch = arm64
>> +extra_params = -append 'event-counter-config'
>> +
>> +[pmu-basic-event-count]
>> +file = pmu.flat
>> +groups = pmu
>> +arch = arm64
>> +extra_params = -append 'basic-event-count'
>> +
>> +[pmu-mem-access]
>> +file = pmu.flat
>> +groups = pmu
>> +arch = arm64
>> +extra_params = -append 'mem-access'
>> +
>>  # Test PMU support (TCG) with -icount IPC=1
>>  #[pmu-tcg-icount-1]
>>  #file = pmu.flat
>> -- 
>> 2.20.1
>>
>>
> 
> I only skimmed this looking for style/framework issues. I'd prefer more
> defines in general to all the hex, but I won't insist on that.
> 
> Thanks,
> drew
> 

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [kvm-unit-tests PATCH v2 5/9] arm: pmu: Basic event counter Tests

[Andrew Jones]

On Thu, Jan 30, 2020 at 12:25:06PM +0100, Eric Auger wrote:
> Adds the following tests:
> - event-counter-config: test event counter configuration
> - basic-event-count:
>   - programs counters #0 and #1 to count 2 required events
>   (resp. CPU_CYCLES and INST_RETIRED). Counter #0 is preset
>   to a value close enough to the 32b
>   overflow limit so that we check the overflow bit is set
>   after the execution of the asm loop.
> - mem-access: counts MEM_ACCESS event on counters #0 and #1
>   with and without 32-bit overflow.
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> 
> ---
> 
> v1 -> v2:
> - fix PMCNTENSET_EL0 and PMCNTENCLR_EL0 op0
> - print PMEVTYPER SH
> - properly clobber used regs and add "cc"
> - simplify mem_access_loop
> ---
>  arm/pmu.c         | 269 ++++++++++++++++++++++++++++++++++++++++++++++
>  arm/unittests.cfg |  18 ++++
>  2 files changed, 287 insertions(+)
> 
> diff --git a/arm/pmu.c b/arm/pmu.c
> index 4a26a76..1b0101f 100644
> --- a/arm/pmu.c
> +++ b/arm/pmu.c
> @@ -18,9 +18,15 @@
>  #include "asm/barrier.h"
>  #include "asm/sysreg.h"
>  #include "asm/processor.h"
> +#include <bitops.h>
> +#include <asm/gic.h>
>  
>  #define PMU_PMCR_E         (1 << 0)
> +#define PMU_PMCR_P         (1 << 1)
>  #define PMU_PMCR_C         (1 << 2)
> +#define PMU_PMCR_D         (1 << 3)
> +#define PMU_PMCR_X         (1 << 4)
> +#define PMU_PMCR_DP        (1 << 5)
>  #define PMU_PMCR_LC        (1 << 6)
>  #define PMU_PMCR_N_SHIFT   11
>  #define PMU_PMCR_N_MASK    0x1f
> @@ -104,6 +110,9 @@ static inline void precise_instrs_loop(int loop, uint32_t pmcr)
>  
>  /* event counter tests only implemented for aarch64 */
>  static void test_event_introspection(void) {}
> +static void test_event_counter_config(void) {}
> +static void test_basic_event_count(void) {}
> +static void test_mem_access(void) {}
>  
>  #elif defined(__aarch64__)
>  #define ID_AA64DFR0_PERFMON_SHIFT 8
> @@ -145,6 +154,33 @@ static inline void precise_instrs_loop(int loop, uint32_t pmcr)
>  }
>  
>  #define PMCEID1_EL0 sys_reg(3, 3, 9, 12, 7)
> +#define PMCNTENSET_EL0 sys_reg(3, 3, 9, 12, 1)
> +#define PMCNTENCLR_EL0 sys_reg(3, 3, 9, 12, 2)
> +
> +#define PMEVTYPER_EXCLUDE_EL1 (1 << 31)
> +#define PMEVTYPER_EXCLUDE_EL0 (1 << 30)
> +
> +#define regn_el0(__reg, __n) __reg ## __n  ## _el0
> +#define write_regn(__reg, __n, __val) \
> +	write_sysreg((__val), __reg ## __n ## _el0)
> +
> +#define read_regn(__reg, __n) \
> +	read_sysreg(__reg ## __n ## _el0)
> +
> +#define print_pmevtyper(__s, __n) do { \
> +	uint32_t val; \
> +	val = read_regn(pmevtyper, __n);\
> +	report_info("%s pmevtyper%d=0x%x, eventcount=0x%x (p=%ld, u=%ld nsk=%ld, nsu=%ld, nsh=%ld m=%ld, mt=%ld, sh=%ld)", \
> +			(__s), (__n), val, val & 0xFFFF,	\
> +			(BIT_MASK(31) & val) >> 31,		\
> +			(BIT_MASK(30) & val) >> 30,		\
> +			(BIT_MASK(29) & val) >> 29,		\
> +			(BIT_MASK(28) & val) >> 28,		\
> +			(BIT_MASK(27) & val) >> 27,		\
> +			(BIT_MASK(26) & val) >> 26,		\
> +			(BIT_MASK(25) & val) >> 25);		\
> +			(BIT_MASK(24) & val) >> 24);		\
> +	} while (0)
>  
>  static bool is_event_supported(uint32_t n, bool warn)
>  {
> @@ -198,6 +234,230 @@ static void test_event_introspection(void)
>  	report(required_events, "Check required events are implemented");
>  }
>  
> +/*
> + * 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. Loads @loop times the data at @address into x9.
> + */
> +static void mem_access_loop(void *addr, int loop, uint32_t pmcr)
> +{
> +asm volatile(
> +	"       msr     pmcr_el0, %[pmcr]\n"
> +	"       isb\n"
> +	"       mov     x10, %[loop]\n"
> +	"1:     sub     x10, x10, #1\n"
> +	"       ldr x9, [%[addr]]\n"
> +	"       cmp     x10, #0x0\n"
> +	"       b.gt    1b\n"
> +	"       msr     pmcr_el0, xzr\n"
> +	"       isb\n"
> +	:
> +	: [addr] "r" (addr), [pmcr] "r" (pmcr), [loop] "r" (loop)
> +	: "x9", "x10", "cc");
> +}
> +
> +static void pmu_reset(void)
> +{
> +	/* reset all counters, counting disabled at PMCR level*/
> +	set_pmcr(pmu.pmcr_ro | PMU_PMCR_LC | PMU_PMCR_C | PMU_PMCR_P);
> +	/* Disable all counters */
> +	write_sysreg_s(0xFFFFFFFF, PMCNTENCLR_EL0);
> +	/* clear overflow reg */
> +	write_sysreg(0xFFFFFFFF, pmovsclr_el0);
> +	/* disable overflow interrupts on all counters */
> +	write_sysreg(0xFFFFFFFF, pmintenclr_el1);
> +	isb();

Is there a test we can do here to ensure the PMU was succesfully reset and
assert if not? Reset is a critical part of each test prep, so we should be
sure it has been done.

> +}
> +
> +static void test_event_counter_config(void)
> +{
> +	int i;
> +
> +	if (!pmu.nb_implemented_counters) {
> +		report_skip("No event counter, skip ...");
> +		return;
> +	}
> +
> +	pmu_reset();
> +
> +	/*
> +	 * Test setting through PMESELR/PMXEVTYPER and PMEVTYPERn read,
> +	 * select counter 0
> +	 */
> +	write_sysreg(1, PMSELR_EL0);
> +	/* program this counter to count unsupported event */
> +	write_sysreg(0xEA, PMXEVTYPER_EL0);
> +	write_sysreg(0xdeadbeef, PMXEVCNTR_EL0);
> +	report((read_regn(pmevtyper, 1) & 0xFFF) == 0xEA,
> +		"PMESELR/PMXEVTYPER/PMEVTYPERn");
> +	report((read_regn(pmevcntr, 1) == 0xdeadbeef),
> +		"PMESELR/PMXEVCNTR/PMEVCNTRn");
> +
> +	/* try to configure an unsupported event within the range [0x0, 0x3F] */
> +	for (i = 0; i <= 0x3F; i++) {
> +		if (!is_event_supported(i, false))
> +			break;
> +	}
> +	if (i > 0x3F) {
> +		report_skip("pmevtyper: all events within [0x0, 0x3F] are supported");
> +		return;
> +	}
> +
> +	/* select counter 0 */
> +	write_sysreg(0, PMSELR_EL0);
> +	/* program this counter to count unsupported event */
> +	write_sysreg(i, PMXEVCNTR_EL0);
> +	/* read the counter value */
> +	read_sysreg(PMXEVCNTR_EL0);
> +	report(read_sysreg(PMXEVCNTR_EL0) == i,
> +		"read of a counter programmed with unsupported event");
> +
> +}
> +
> +static bool satisfy_prerequisites(uint32_t *events, unsigned int nb_events)
> +{
> +	int i;
> +
> +	if (pmu.nb_implemented_counters < nb_events) {
> +		report_skip("Skip test as number of counters is too small (%d)",
> +			    pmu.nb_implemented_counters);
> +		return false;
> +	}
> +
> +	for (i = 0; i < nb_events; i++) {
> +		if (!is_event_supported(events[i], false)) {
> +			report_skip("Skip test as event %d is not supported",
> +				    events[i]);
> +			return false;
> +		}
> +	}
> +	return true;
> +}
> +
> +static void test_basic_event_count(void)
> +{
> +	uint32_t implemented_counter_mask, non_implemented_counter_mask;
> +	uint32_t counter_mask;
> +	uint32_t events[] = {
> +		0x11,	/* CPU_CYCLES */
> +		0x8,	/* INST_RETIRED */
> +	};
> +
> +	if (!satisfy_prerequisites(events, ARRAY_SIZE(events)))
> +		return;
> +
> +	implemented_counter_mask = BIT(pmu.nb_implemented_counters) - 1;
> +	non_implemented_counter_mask = ~(BIT(31) | implemented_counter_mask);
> +	counter_mask = implemented_counter_mask | non_implemented_counter_mask;
> +
> +	write_regn(pmevtyper, 0, events[0] | PMEVTYPER_EXCLUDE_EL0);
> +	write_regn(pmevtyper, 1, events[1] | PMEVTYPER_EXCLUDE_EL0);
> +
> +	/* disable all counters */
> +	write_sysreg_s(0xFFFFFFFF, PMCNTENCLR_EL0);
> +	report(!read_sysreg_s(PMCNTENCLR_EL0) && !read_sysreg_s(PMCNTENSET_EL0),
> +		"pmcntenclr: disable all counters");
> +
> +	/*
> +	 * clear cycle and all event counters and allow counter enablement
> +	 * through PMCNTENSET. LC is RES1.
> +	 */
> +	set_pmcr(pmu.pmcr_ro | PMU_PMCR_LC | PMU_PMCR_C | PMU_PMCR_P);
> +	isb();
> +	report(get_pmcr() == (pmu.pmcr_ro | PMU_PMCR_LC), "pmcr: reset counters");
> +
> +	/* Preset counter #0 to 0xFFFFFFF0 to trigger an overflow interrupt */
> +	write_regn(pmevcntr, 0, 0xFFFFFFF0);
> +	report(read_regn(pmevcntr, 0) == 0xFFFFFFF0,
> +		"counter #0 preset to 0xFFFFFFF0");
> +	report(!read_regn(pmevcntr, 1), "counter #1 is 0");
> +
> +	/*
> +	 * Enable all implemented counters and also attempt to enable
> +	 * not supported counters. Counting still is disabled by !PMCR.E
> +	 */
> +	write_sysreg_s(counter_mask, PMCNTENSET_EL0);
> +
> +	/* check only those implemented are enabled */
> +	report((read_sysreg_s(PMCNTENSET_EL0) == read_sysreg_s(PMCNTENCLR_EL0)) &&
> +		(read_sysreg_s(PMCNTENSET_EL0) == implemented_counter_mask),
> +		"pmcntenset: enabled implemented_counters");
> +
> +	/* Disable all counters but counters #0 and #1 */
> +	write_sysreg_s(~0x3, PMCNTENCLR_EL0);
> +	report((read_sysreg_s(PMCNTENSET_EL0) == read_sysreg_s(PMCNTENCLR_EL0)) &&
> +		(read_sysreg_s(PMCNTENSET_EL0) == 0x3),
> +		"pmcntenset: just enabled #0 and #1");
> +
> +	/* clear overflow register */
> +	write_sysreg(0xFFFFFFFF, pmovsclr_el0);
> +	report(!read_sysreg(pmovsclr_el0), "check overflow reg is 0");
> +
> +	/* disable overflow interrupts on all counters*/
> +	write_sysreg(0xFFFFFFFF, pmintenclr_el1);
> +	report(!read_sysreg(pmintenclr_el1),
> +		"pmintenclr_el1=0, all interrupts disabled");
> +
> +	/* enable overflow interrupts on all event counters */
> +	write_sysreg(implemented_counter_mask | non_implemented_counter_mask,
> +		     pmintenset_el1);
> +	report(read_sysreg(pmintenset_el1) == implemented_counter_mask,
> +		"overflow interrupts enabled on all implemented counters");
> +
> +	/* Set PMCR.E, execute asm code and unset PMCR.E */
> +	precise_instrs_loop(20, pmu.pmcr_ro | PMU_PMCR_E);
> +
> +	report_info("counter #0 is 0x%lx (CPU_CYCLES)",
> +		    read_regn(pmevcntr, 0));
> +	report_info("counter #1 is 0x%lx (INST_RETIRED)",
> +		    read_regn(pmevcntr, 1));
> +
> +	report_info("overflow reg = 0x%lx", read_sysreg(pmovsclr_el0));
> +	report(read_sysreg(pmovsclr_el0) & 0x1,
> +		"check overflow happened on #0 only");
> +}
> +
> +static void test_mem_access(void)
> +{
> +	void *addr = malloc(PAGE_SIZE);
> +	uint32_t events[] = {
> +		0x13,   /* MEM_ACCESS */
> +		0x13,   /* MEM_ACCESS */
> +	};
> +
> +	if (!satisfy_prerequisites(events, ARRAY_SIZE(events)))
> +		return;
> +
> +	pmu_reset();
> +
> +	write_regn(pmevtyper, 0, events[0] | PMEVTYPER_EXCLUDE_EL0);
> +	write_regn(pmevtyper, 1, events[1] | PMEVTYPER_EXCLUDE_EL0);
> +	write_sysreg_s(0x3, PMCNTENSET_EL0);
> +	isb();
> +	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E);
> +	report_info("counter #0 is %ld (MEM_ACCESS)", read_regn(pmevcntr, 0));
> +	report_info("counter #1 is %ld (MEM_ACCESS)", read_regn(pmevcntr, 1));
> +	/* We may measure more than 20 mem access depending on the core */
> +	report((read_regn(pmevcntr, 0) == read_regn(pmevcntr, 1)) &&
> +	       (read_regn(pmevcntr, 0) >= 20) && !read_sysreg(pmovsclr_el0),
> +	       "Ran 20 mem accesses");
> +
> +	pmu_reset();
> +
> +	write_regn(pmevcntr, 0, 0xFFFFFFFA);
> +	write_regn(pmevcntr, 1, 0xFFFFFFF0);
> +	write_sysreg_s(0x3, PMCNTENSET_EL0);
> +	isb();
> +	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E);
> +	report(read_sysreg(pmovsclr_el0) == 0x3,
> +	       "Ran 20 mem accesses with expected overflows on both counters");
> +	report_info("cnt#0 = %ld cnt#1=%ld overflow=0x%lx",
> +			read_regn(pmevcntr, 0), read_regn(pmevcntr, 1),
> +			read_sysreg(pmovsclr_el0));
> +}
> +
>  #endif
>  
>  /*
> @@ -388,6 +648,15 @@ int main(int argc, char *argv[])
>  	} else if (strcmp(argv[1], "event-introspection") == 0) {
>  		report_prefix_push(argv[1]);
>  		test_event_introspection();
> +	} else if (strcmp(argv[1], "event-counter-config") == 0) {
> +		report_prefix_push(argv[1]);
> +		test_event_counter_config();
> +	} else if (strcmp(argv[1], "basic-event-count") == 0) {
> +		report_prefix_push(argv[1]);
> +		test_basic_event_count();
> +	} else if (strcmp(argv[1], "mem-access") == 0) {
> +		report_prefix_push(argv[1]);
> +		test_mem_access();
>  	} else {
>  		report_abort("Unknown sub-test '%s'", argv[1]);
>  	}
> diff --git a/arm/unittests.cfg b/arm/unittests.cfg
> index 4433ef3..7a59403 100644
> --- a/arm/unittests.cfg
> +++ b/arm/unittests.cfg
> @@ -72,6 +72,24 @@ groups = pmu
>  arch = arm64
>  extra_params = -append 'event-introspection'
>  
> +[pmu-event-counter-config]
> +file = pmu.flat
> +groups = pmu
> +arch = arm64
> +extra_params = -append 'event-counter-config'
> +
> +[pmu-basic-event-count]
> +file = pmu.flat
> +groups = pmu
> +arch = arm64
> +extra_params = -append 'basic-event-count'
> +
> +[pmu-mem-access]
> +file = pmu.flat
> +groups = pmu
> +arch = arm64
> +extra_params = -append 'mem-access'
> +
>  # Test PMU support (TCG) with -icount IPC=1
>  #[pmu-tcg-icount-1]
>  #file = pmu.flat
> -- 
> 2.20.1
> 
> 

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [kvm-unit-tests PATCH v2 5/9] arm: pmu: Basic event counter Tests

[Auger Eric]

Hi Drew,

On 3/5/20 10:42 AM, Andrew Jones wrote:
> On Thu, Jan 30, 2020 at 12:25:06PM +0100, Eric Auger wrote:
>> Adds the following tests:
>> - event-counter-config: test event counter configuration
>> - basic-event-count:
>>   - programs counters #0 and #1 to count 2 required events
>>   (resp. CPU_CYCLES and INST_RETIRED). Counter #0 is preset
>>   to a value close enough to the 32b
>>   overflow limit so that we check the overflow bit is set
>>   after the execution of the asm loop.
>> - mem-access: counts MEM_ACCESS event on counters #0 and #1
>>   with and without 32-bit overflow.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>
>> ---
>>
>> v1 -> v2:
>> - fix PMCNTENSET_EL0 and PMCNTENCLR_EL0 op0
>> - print PMEVTYPER SH
>> - properly clobber used regs and add "cc"
>> - simplify mem_access_loop
>> ---
>>  arm/pmu.c         | 269 ++++++++++++++++++++++++++++++++++++++++++++++
>>  arm/unittests.cfg |  18 ++++
>>  2 files changed, 287 insertions(+)
>>
>> diff --git a/arm/pmu.c b/arm/pmu.c
>> index 4a26a76..1b0101f 100644
>> --- a/arm/pmu.c
>> +++ b/arm/pmu.c
>> @@ -18,9 +18,15 @@
>>  #include "asm/barrier.h"
>>  #include "asm/sysreg.h"
>>  #include "asm/processor.h"
>> +#include <bitops.h>
>> +#include <asm/gic.h>
>>  
>>  #define PMU_PMCR_E         (1 << 0)
>> +#define PMU_PMCR_P         (1 << 1)
>>  #define PMU_PMCR_C         (1 << 2)
>> +#define PMU_PMCR_D         (1 << 3)
>> +#define PMU_PMCR_X         (1 << 4)
>> +#define PMU_PMCR_DP        (1 << 5)
>>  #define PMU_PMCR_LC        (1 << 6)
>>  #define PMU_PMCR_N_SHIFT   11
>>  #define PMU_PMCR_N_MASK    0x1f
>> @@ -104,6 +110,9 @@ static inline void precise_instrs_loop(int loop, uint32_t pmcr)
>>  
>>  /* event counter tests only implemented for aarch64 */
>>  static void test_event_introspection(void) {}
>> +static void test_event_counter_config(void) {}
>> +static void test_basic_event_count(void) {}
>> +static void test_mem_access(void) {}
>>  
>>  #elif defined(__aarch64__)
>>  #define ID_AA64DFR0_PERFMON_SHIFT 8
>> @@ -145,6 +154,33 @@ static inline void precise_instrs_loop(int loop, uint32_t pmcr)
>>  }
>>  
>>  #define PMCEID1_EL0 sys_reg(3, 3, 9, 12, 7)
>> +#define PMCNTENSET_EL0 sys_reg(3, 3, 9, 12, 1)
>> +#define PMCNTENCLR_EL0 sys_reg(3, 3, 9, 12, 2)
>> +
>> +#define PMEVTYPER_EXCLUDE_EL1 (1 << 31)
>> +#define PMEVTYPER_EXCLUDE_EL0 (1 << 30)
>> +
>> +#define regn_el0(__reg, __n) __reg ## __n  ## _el0
>> +#define write_regn(__reg, __n, __val) \
>> +	write_sysreg((__val), __reg ## __n ## _el0)
>> +
>> +#define read_regn(__reg, __n) \
>> +	read_sysreg(__reg ## __n ## _el0)
>> +
>> +#define print_pmevtyper(__s, __n) do { \
>> +	uint32_t val; \
>> +	val = read_regn(pmevtyper, __n);\
>> +	report_info("%s pmevtyper%d=0x%x, eventcount=0x%x (p=%ld, u=%ld nsk=%ld, nsu=%ld, nsh=%ld m=%ld, mt=%ld, sh=%ld)", \
>> +			(__s), (__n), val, val & 0xFFFF,	\
>> +			(BIT_MASK(31) & val) >> 31,		\
>> +			(BIT_MASK(30) & val) >> 30,		\
>> +			(BIT_MASK(29) & val) >> 29,		\
>> +			(BIT_MASK(28) & val) >> 28,		\
>> +			(BIT_MASK(27) & val) >> 27,		\
>> +			(BIT_MASK(26) & val) >> 26,		\
>> +			(BIT_MASK(25) & val) >> 25);		\
>> +			(BIT_MASK(24) & val) >> 24);		\
>> +	} while (0)
>>  
>>  static bool is_event_supported(uint32_t n, bool warn)
>>  {
>> @@ -198,6 +234,230 @@ static void test_event_introspection(void)
>>  	report(required_events, "Check required events are implemented");
>>  }
>>  
>> +/*
>> + * 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. Loads @loop times the data at @address into x9.
>> + */
>> +static void mem_access_loop(void *addr, int loop, uint32_t pmcr)
>> +{
>> +asm volatile(
>> +	"       msr     pmcr_el0, %[pmcr]\n"
>> +	"       isb\n"
>> +	"       mov     x10, %[loop]\n"
>> +	"1:     sub     x10, x10, #1\n"
>> +	"       ldr x9, [%[addr]]\n"
>> +	"       cmp     x10, #0x0\n"
>> +	"       b.gt    1b\n"
>> +	"       msr     pmcr_el0, xzr\n"
>> +	"       isb\n"
>> +	:
>> +	: [addr] "r" (addr), [pmcr] "r" (pmcr), [loop] "r" (loop)
>> +	: "x9", "x10", "cc");
>> +}
>> +
>> +static void pmu_reset(void)
>> +{
>> +	/* reset all counters, counting disabled at PMCR level*/
>> +	set_pmcr(pmu.pmcr_ro | PMU_PMCR_LC | PMU_PMCR_C | PMU_PMCR_P);
>> +	/* Disable all counters */
>> +	write_sysreg_s(0xFFFFFFFF, PMCNTENCLR_EL0);
>> +	/* clear overflow reg */
>> +	write_sysreg(0xFFFFFFFF, pmovsclr_el0);
>> +	/* disable overflow interrupts on all counters */
>> +	write_sysreg(0xFFFFFFFF, pmintenclr_el1);
>> +	isb();
> 
> Is there a test we can do here to ensure the PMU was succesfully reset and
> assert if not? Reset is a critical part of each test prep, so we should be
> sure it has been done.
we can read the regs we have just written (?). In case the reset were to
fail, the subsequent test would fail anyway.

Thanks

Eric
> 
>> +}
>> +
>> +static void test_event_counter_config(void)
>> +{
>> +	int i;
>> +
>> +	if (!pmu.nb_implemented_counters) {
>> +		report_skip("No event counter, skip ...");
>> +		return;
>> +	}
>> +
>> +	pmu_reset();
>> +
>> +	/*
>> +	 * Test setting through PMESELR/PMXEVTYPER and PMEVTYPERn read,
>> +	 * select counter 0
>> +	 */
>> +	write_sysreg(1, PMSELR_EL0);
>> +	/* program this counter to count unsupported event */
>> +	write_sysreg(0xEA, PMXEVTYPER_EL0);
>> +	write_sysreg(0xdeadbeef, PMXEVCNTR_EL0);
>> +	report((read_regn(pmevtyper, 1) & 0xFFF) == 0xEA,
>> +		"PMESELR/PMXEVTYPER/PMEVTYPERn");
>> +	report((read_regn(pmevcntr, 1) == 0xdeadbeef),
>> +		"PMESELR/PMXEVCNTR/PMEVCNTRn");
>> +
>> +	/* try to configure an unsupported event within the range [0x0, 0x3F] */
>> +	for (i = 0; i <= 0x3F; i++) {
>> +		if (!is_event_supported(i, false))
>> +			break;
>> +	}
>> +	if (i > 0x3F) {
>> +		report_skip("pmevtyper: all events within [0x0, 0x3F] are supported");
>> +		return;
>> +	}
>> +
>> +	/* select counter 0 */
>> +	write_sysreg(0, PMSELR_EL0);
>> +	/* program this counter to count unsupported event */
>> +	write_sysreg(i, PMXEVCNTR_EL0);
>> +	/* read the counter value */
>> +	read_sysreg(PMXEVCNTR_EL0);
>> +	report(read_sysreg(PMXEVCNTR_EL0) == i,
>> +		"read of a counter programmed with unsupported event");
>> +
>> +}
>> +
>> +static bool satisfy_prerequisites(uint32_t *events, unsigned int nb_events)
>> +{
>> +	int i;
>> +
>> +	if (pmu.nb_implemented_counters < nb_events) {
>> +		report_skip("Skip test as number of counters is too small (%d)",
>> +			    pmu.nb_implemented_counters);
>> +		return false;
>> +	}
>> +
>> +	for (i = 0; i < nb_events; i++) {
>> +		if (!is_event_supported(events[i], false)) {
>> +			report_skip("Skip test as event %d is not supported",
>> +				    events[i]);
>> +			return false;
>> +		}
>> +	}
>> +	return true;
>> +}
>> +
>> +static void test_basic_event_count(void)
>> +{
>> +	uint32_t implemented_counter_mask, non_implemented_counter_mask;
>> +	uint32_t counter_mask;
>> +	uint32_t events[] = {
>> +		0x11,	/* CPU_CYCLES */
>> +		0x8,	/* INST_RETIRED */
>> +	};
>> +
>> +	if (!satisfy_prerequisites(events, ARRAY_SIZE(events)))
>> +		return;
>> +
>> +	implemented_counter_mask = BIT(pmu.nb_implemented_counters) - 1;
>> +	non_implemented_counter_mask = ~(BIT(31) | implemented_counter_mask);
>> +	counter_mask = implemented_counter_mask | non_implemented_counter_mask;
>> +
>> +	write_regn(pmevtyper, 0, events[0] | PMEVTYPER_EXCLUDE_EL0);
>> +	write_regn(pmevtyper, 1, events[1] | PMEVTYPER_EXCLUDE_EL0);
>> +
>> +	/* disable all counters */
>> +	write_sysreg_s(0xFFFFFFFF, PMCNTENCLR_EL0);
>> +	report(!read_sysreg_s(PMCNTENCLR_EL0) && !read_sysreg_s(PMCNTENSET_EL0),
>> +		"pmcntenclr: disable all counters");
>> +
>> +	/*
>> +	 * clear cycle and all event counters and allow counter enablement
>> +	 * through PMCNTENSET. LC is RES1.
>> +	 */
>> +	set_pmcr(pmu.pmcr_ro | PMU_PMCR_LC | PMU_PMCR_C | PMU_PMCR_P);
>> +	isb();
>> +	report(get_pmcr() == (pmu.pmcr_ro | PMU_PMCR_LC), "pmcr: reset counters");
>> +
>> +	/* Preset counter #0 to 0xFFFFFFF0 to trigger an overflow interrupt */
>> +	write_regn(pmevcntr, 0, 0xFFFFFFF0);
>> +	report(read_regn(pmevcntr, 0) == 0xFFFFFFF0,
>> +		"counter #0 preset to 0xFFFFFFF0");
>> +	report(!read_regn(pmevcntr, 1), "counter #1 is 0");
>> +
>> +	/*
>> +	 * Enable all implemented counters and also attempt to enable
>> +	 * not supported counters. Counting still is disabled by !PMCR.E
>> +	 */
>> +	write_sysreg_s(counter_mask, PMCNTENSET_EL0);
>> +
>> +	/* check only those implemented are enabled */
>> +	report((read_sysreg_s(PMCNTENSET_EL0) == read_sysreg_s(PMCNTENCLR_EL0)) &&
>> +		(read_sysreg_s(PMCNTENSET_EL0) == implemented_counter_mask),
>> +		"pmcntenset: enabled implemented_counters");
>> +
>> +	/* Disable all counters but counters #0 and #1 */
>> +	write_sysreg_s(~0x3, PMCNTENCLR_EL0);
>> +	report((read_sysreg_s(PMCNTENSET_EL0) == read_sysreg_s(PMCNTENCLR_EL0)) &&
>> +		(read_sysreg_s(PMCNTENSET_EL0) == 0x3),
>> +		"pmcntenset: just enabled #0 and #1");
>> +
>> +	/* clear overflow register */
>> +	write_sysreg(0xFFFFFFFF, pmovsclr_el0);
>> +	report(!read_sysreg(pmovsclr_el0), "check overflow reg is 0");
>> +
>> +	/* disable overflow interrupts on all counters*/
>> +	write_sysreg(0xFFFFFFFF, pmintenclr_el1);
>> +	report(!read_sysreg(pmintenclr_el1),
>> +		"pmintenclr_el1=0, all interrupts disabled");
>> +
>> +	/* enable overflow interrupts on all event counters */
>> +	write_sysreg(implemented_counter_mask | non_implemented_counter_mask,
>> +		     pmintenset_el1);
>> +	report(read_sysreg(pmintenset_el1) == implemented_counter_mask,
>> +		"overflow interrupts enabled on all implemented counters");
>> +
>> +	/* Set PMCR.E, execute asm code and unset PMCR.E */
>> +	precise_instrs_loop(20, pmu.pmcr_ro | PMU_PMCR_E);
>> +
>> +	report_info("counter #0 is 0x%lx (CPU_CYCLES)",
>> +		    read_regn(pmevcntr, 0));
>> +	report_info("counter #1 is 0x%lx (INST_RETIRED)",
>> +		    read_regn(pmevcntr, 1));
>> +
>> +	report_info("overflow reg = 0x%lx", read_sysreg(pmovsclr_el0));
>> +	report(read_sysreg(pmovsclr_el0) & 0x1,
>> +		"check overflow happened on #0 only");
>> +}
>> +
>> +static void test_mem_access(void)
>> +{
>> +	void *addr = malloc(PAGE_SIZE);
>> +	uint32_t events[] = {
>> +		0x13,   /* MEM_ACCESS */
>> +		0x13,   /* MEM_ACCESS */
>> +	};
>> +
>> +	if (!satisfy_prerequisites(events, ARRAY_SIZE(events)))
>> +		return;
>> +
>> +	pmu_reset();
>> +
>> +	write_regn(pmevtyper, 0, events[0] | PMEVTYPER_EXCLUDE_EL0);
>> +	write_regn(pmevtyper, 1, events[1] | PMEVTYPER_EXCLUDE_EL0);
>> +	write_sysreg_s(0x3, PMCNTENSET_EL0);
>> +	isb();
>> +	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E);
>> +	report_info("counter #0 is %ld (MEM_ACCESS)", read_regn(pmevcntr, 0));
>> +	report_info("counter #1 is %ld (MEM_ACCESS)", read_regn(pmevcntr, 1));
>> +	/* We may measure more than 20 mem access depending on the core */
>> +	report((read_regn(pmevcntr, 0) == read_regn(pmevcntr, 1)) &&
>> +	       (read_regn(pmevcntr, 0) >= 20) && !read_sysreg(pmovsclr_el0),
>> +	       "Ran 20 mem accesses");
>> +
>> +	pmu_reset();
>> +
>> +	write_regn(pmevcntr, 0, 0xFFFFFFFA);
>> +	write_regn(pmevcntr, 1, 0xFFFFFFF0);
>> +	write_sysreg_s(0x3, PMCNTENSET_EL0);
>> +	isb();
>> +	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E);
>> +	report(read_sysreg(pmovsclr_el0) == 0x3,
>> +	       "Ran 20 mem accesses with expected overflows on both counters");
>> +	report_info("cnt#0 = %ld cnt#1=%ld overflow=0x%lx",
>> +			read_regn(pmevcntr, 0), read_regn(pmevcntr, 1),
>> +			read_sysreg(pmovsclr_el0));
>> +}
>> +
>>  #endif
>>  
>>  /*
>> @@ -388,6 +648,15 @@ int main(int argc, char *argv[])
>>  	} else if (strcmp(argv[1], "event-introspection") == 0) {
>>  		report_prefix_push(argv[1]);
>>  		test_event_introspection();
>> +	} else if (strcmp(argv[1], "event-counter-config") == 0) {
>> +		report_prefix_push(argv[1]);
>> +		test_event_counter_config();
>> +	} else if (strcmp(argv[1], "basic-event-count") == 0) {
>> +		report_prefix_push(argv[1]);
>> +		test_basic_event_count();
>> +	} else if (strcmp(argv[1], "mem-access") == 0) {
>> +		report_prefix_push(argv[1]);
>> +		test_mem_access();
>>  	} else {
>>  		report_abort("Unknown sub-test '%s'", argv[1]);
>>  	}
>> diff --git a/arm/unittests.cfg b/arm/unittests.cfg
>> index 4433ef3..7a59403 100644
>> --- a/arm/unittests.cfg
>> +++ b/arm/unittests.cfg
>> @@ -72,6 +72,24 @@ groups = pmu
>>  arch = arm64
>>  extra_params = -append 'event-introspection'
>>  
>> +[pmu-event-counter-config]
>> +file = pmu.flat
>> +groups = pmu
>> +arch = arm64
>> +extra_params = -append 'event-counter-config'
>> +
>> +[pmu-basic-event-count]
>> +file = pmu.flat
>> +groups = pmu
>> +arch = arm64
>> +extra_params = -append 'basic-event-count'
>> +
>> +[pmu-mem-access]
>> +file = pmu.flat
>> +groups = pmu
>> +arch = arm64
>> +extra_params = -append 'mem-access'
>> +
>>  # Test PMU support (TCG) with -icount IPC=1
>>  #[pmu-tcg-icount-1]
>>  #file = pmu.flat
>> -- 
>> 2.20.1
>>
>>

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[kvm-unit-tests PATCH v2 6/9] arm: pmu: Test chained counter

[Eric Auger]

Add 2 tests exercising chained counters. The first one uses
CPU_CYCLES and the second one uses SW_INCR.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 arm/pmu.c         | 128 ++++++++++++++++++++++++++++++++++++++++++++++
 arm/unittests.cfg |  12 +++++
 2 files changed, 140 insertions(+)

diff --git a/arm/pmu.c b/arm/pmu.c
index 1b0101f..538fbeb 100644
--- a/arm/pmu.c
+++ b/arm/pmu.c
@@ -113,6 +113,8 @@ static void test_event_introspection(void) {}
 static void test_event_counter_config(void) {}
 static void test_basic_event_count(void) {}
 static void test_mem_access(void) {}
+static void test_chained_counters(void) {}
+static void test_chained_sw_incr(void) {}
 
 #elif defined(__aarch64__)
 #define ID_AA64DFR0_PERFMON_SHIFT 8
@@ -458,6 +460,126 @@ static void test_mem_access(void)
 			read_sysreg(pmovsclr_el0));
 }
 
+static void test_chained_counters(void)
+{
+	uint32_t events[] = { 0x11 /* CPU_CYCLES */, 0x1E /* CHAIN */};
+
+	if (!satisfy_prerequisites(events, ARRAY_SIZE(events)))
+		return;
+
+	pmu_reset();
+
+	write_regn(pmevtyper, 0, events[0] | PMEVTYPER_EXCLUDE_EL0);
+	write_regn(pmevtyper, 1, events[1] | PMEVTYPER_EXCLUDE_EL0);
+	/* enable counters #0 and #1 */
+	write_sysreg_s(0x3, PMCNTENSET_EL0);
+	/* preset counter #0 at 0xFFFFFFF0 */
+	write_regn(pmevcntr, 0, 0xFFFFFFF0);
+
+	precise_instrs_loop(22, pmu.pmcr_ro | PMU_PMCR_E);
+
+	report(read_regn(pmevcntr, 1) == 1, "CHAIN counter #1 incremented");
+	report(!read_sysreg(pmovsclr_el0), "check no overflow is recorded");
+
+	/* test 64b overflow */
+
+	pmu_reset();
+	write_sysreg_s(0x3, PMCNTENSET_EL0);
+
+	write_regn(pmevcntr, 0, 0xFFFFFFF0);
+	write_regn(pmevcntr, 1, 0x1);
+	precise_instrs_loop(22, pmu.pmcr_ro | PMU_PMCR_E);
+	report_info("overflow reg = 0x%lx", read_sysreg(pmovsclr_el0));
+	report(read_regn(pmevcntr, 1) == 2, "CHAIN counter #1 incremented");
+	report(!read_sysreg(pmovsclr_el0), "check no overflow is recorded");
+
+	write_regn(pmevcntr, 0, 0xFFFFFFF0);
+	write_regn(pmevcntr, 1, 0xFFFFFFFF);
+
+	precise_instrs_loop(22, pmu.pmcr_ro | PMU_PMCR_E);
+	report_info("overflow reg = 0x%lx", read_sysreg(pmovsclr_el0));
+	report(!read_regn(pmevcntr, 1), "CHAIN counter #1 wrapped");
+	report(read_sysreg(pmovsclr_el0) == 0x2,
+		"check no overflow is recorded");
+}
+
+static void test_chained_sw_incr(void)
+{
+	uint32_t events[] = { 0x0 /* SW_INCR */, 0x0 /* SW_INCR */};
+	int i;
+
+	if (!satisfy_prerequisites(events, ARRAY_SIZE(events)))
+		return;
+
+	pmu_reset();
+
+	write_regn(pmevtyper, 0, events[0] | PMEVTYPER_EXCLUDE_EL0);
+	write_regn(pmevtyper, 1, events[1] | PMEVTYPER_EXCLUDE_EL0);
+	/* enable counters #0 and #1 */
+	write_sysreg_s(0x3, PMCNTENSET_EL0);
+
+	/* preset counter #0 at 0xFFFFFFF0 */
+	write_regn(pmevcntr, 0, 0xFFFFFFF0);
+
+	for (i = 0; i < 100; i++)
+		write_sysreg(0x1, pmswinc_el0);
+
+	report_info("SW_INCR counter #0 has value %ld", read_regn(pmevcntr, 0));
+	report(read_regn(pmevcntr, 0) == 0xFFFFFFF0,
+		"PWSYNC does not increment if PMCR.E is unset");
+
+	pmu_reset();
+
+	write_regn(pmevcntr, 0, 0xFFFFFFF0);
+	write_sysreg_s(0x3, PMCNTENSET_EL0);
+	set_pmcr(pmu.pmcr_ro | PMU_PMCR_E);
+
+	for (i = 0; i < 100; i++)
+		write_sysreg(0x3, pmswinc_el0);
+
+	report(read_regn(pmevcntr, 0)  == 84, "counter #1 after + 100 SW_INCR");
+	report(read_regn(pmevcntr, 1)  == 100,
+		"counter #0 after + 100 SW_INCR");
+	report_info("counter values after 100 SW_INCR #0=%ld #1=%ld",
+		    read_regn(pmevcntr, 0), read_regn(pmevcntr, 1));
+	report(read_sysreg(pmovsclr_el0) == 0x1,
+		"overflow reg after 100 SW_INCR");
+
+	/* 64b SW_INCR */
+	pmu_reset();
+
+	events[1] = 0x1E /* CHAIN */;
+	write_regn(pmevtyper, 1, events[1] | PMEVTYPER_EXCLUDE_EL0);
+	write_regn(pmevcntr, 0, 0xFFFFFFF0);
+	write_sysreg_s(0x3, PMCNTENSET_EL0);
+	set_pmcr(pmu.pmcr_ro | PMU_PMCR_E);
+	for (i = 0; i < 100; i++)
+		write_sysreg(0x3, pmswinc_el0);
+
+	report(!read_sysreg(pmovsclr_el0) && (read_regn(pmevcntr, 1) == 1),
+		"overflow reg after 100 SW_INCR/CHAIN");
+	report_info("overflow=0x%lx, #0=%ld #1=%ld", read_sysreg(pmovsclr_el0),
+		    read_regn(pmevcntr, 0), read_regn(pmevcntr, 1));
+
+	/* 64b SW_INCR and overflow on CHAIN counter*/
+	pmu_reset();
+
+	write_regn(pmevtyper, 1, events[1] | PMEVTYPER_EXCLUDE_EL0);
+	write_regn(pmevcntr, 0, 0xFFFFFFF0);
+	write_regn(pmevcntr, 1, 0xFFFFFFFF);
+	write_sysreg_s(0x3, PMCNTENSET_EL0);
+	set_pmcr(pmu.pmcr_ro | PMU_PMCR_E);
+	for (i = 0; i < 100; i++)
+		write_sysreg(0x3, pmswinc_el0);
+
+	report((read_sysreg(pmovsclr_el0) == 0x2) &&
+		(read_regn(pmevcntr, 1) == 0) &&
+		(read_regn(pmevcntr, 0) == 84),
+		"overflow reg after 100 SW_INCR/CHAIN");
+	report_info("overflow=0x%lx, #0=%ld #1=%ld", read_sysreg(pmovsclr_el0),
+		    read_regn(pmevcntr, 0), read_regn(pmevcntr, 1));
+}
+
 #endif
 
 /*
@@ -657,6 +779,12 @@ int main(int argc, char *argv[])
 	} else if (strcmp(argv[1], "mem-access") == 0) {
 		report_prefix_push(argv[1]);
 		test_mem_access();
+	} else if (strcmp(argv[1], "chained-counters") == 0) {
+		report_prefix_push(argv[1]);
+		test_chained_counters();
+	} else if (strcmp(argv[1], "chained-sw-incr") == 0) {
+		report_prefix_push(argv[1]);
+		test_chained_sw_incr();
 	} else {
 		report_abort("Unknown sub-test '%s'", argv[1]);
 	}
diff --git a/arm/unittests.cfg b/arm/unittests.cfg
index 7a59403..1bd4319 100644
--- a/arm/unittests.cfg
+++ b/arm/unittests.cfg
@@ -90,6 +90,18 @@ groups = pmu
 arch = arm64
 extra_params = -append 'mem-access'
 
+[pmu-chained-counters]
+file = pmu.flat
+groups = pmu
+arch = arm64
+extra_params = -append 'chained-counters'
+
+[pmu-chained-sw-incr]
+file = pmu.flat
+groups = pmu
+arch = arm64
+extra_params = -append 'chained-sw-incr'
+
 # Test PMU support (TCG) with -icount IPC=1
 #[pmu-tcg-icount-1]
 #file = pmu.flat
-- 
2.20.1

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [kvm-unit-tests PATCH v2 6/9] arm: pmu: Test chained counter

[Peter Maydell]

On Thu, 30 Jan 2020 at 11:26, Eric Auger <eric.auger@redhat.com> wrote:
>
> Add 2 tests exercising chained counters. The first one uses
> CPU_CYCLES and the second one uses SW_INCR.
>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> +static void test_chained_sw_incr(void)
> +{
> +       uint32_t events[] = { 0x0 /* SW_INCR */, 0x0 /* SW_INCR */};

Cut-n-paste error? This test relies on the CHAIN event but it
isn't present in this list of events to pass to satisfy_prerequisites(),
so I suspect the second element should be "0x1e /* CHAIN */" ?

(This makes the test fail on QEMU TCG, because we don't implement
CHAIN.)

thanks
-- PMM
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [kvm-unit-tests PATCH v2 6/9] arm: pmu: Test chained counter

[Auger Eric]

Hi Peter,

On 2/11/20 5:24 PM, Peter Maydell wrote:
> On Thu, 30 Jan 2020 at 11:26, Eric Auger <eric.auger@redhat.com> wrote:
>>
>> Add 2 tests exercising chained counters. The first one uses
>> CPU_CYCLES and the second one uses SW_INCR.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> +static void test_chained_sw_incr(void)
>> +{
>> +       uint32_t events[] = { 0x0 /* SW_INCR */, 0x0 /* SW_INCR */};
> 
> Cut-n-paste error? This test relies on the CHAIN event but it
> isn't present in this list of events to pass to satisfy_prerequisites(),
> so I suspect the second element should be "0x1e /* CHAIN */" ?
No that's not a cut-n-paste error. I may rename the test into test_sw_incr.

It starts by testing unchained SW_INCR.

chained SW_INCR testing start with
/* 64b SW_INCR */


> 
> (This makes the test fail on QEMU TCG, because we don't implement
> CHAIN.)
OK

Thanks

Eric
> 
> thanks
> -- PMM
> 

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [kvm-unit-tests PATCH v2 6/9] arm: pmu: Test chained counter

[Andrew Jones]

On Thu, Jan 30, 2020 at 12:25:07PM +0100, Eric Auger wrote:
> Add 2 tests exercising chained counters. The first one uses
> CPU_CYCLES and the second one uses SW_INCR.
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> ---
>  arm/pmu.c         | 128 ++++++++++++++++++++++++++++++++++++++++++++++
>  arm/unittests.cfg |  12 +++++
>  2 files changed, 140 insertions(+)
> 
> diff --git a/arm/pmu.c b/arm/pmu.c
> index 1b0101f..538fbeb 100644
> --- a/arm/pmu.c
> +++ b/arm/pmu.c
> @@ -113,6 +113,8 @@ static void test_event_introspection(void) {}
>  static void test_event_counter_config(void) {}
>  static void test_basic_event_count(void) {}
>  static void test_mem_access(void) {}
> +static void test_chained_counters(void) {}
> +static void test_chained_sw_incr(void) {}
>  
>  #elif defined(__aarch64__)
>  #define ID_AA64DFR0_PERFMON_SHIFT 8
> @@ -458,6 +460,126 @@ static void test_mem_access(void)
>  			read_sysreg(pmovsclr_el0));
>  }
>  
> +static void test_chained_counters(void)
> +{
> +	uint32_t events[] = { 0x11 /* CPU_CYCLES */, 0x1E /* CHAIN */};
> +
> +	if (!satisfy_prerequisites(events, ARRAY_SIZE(events)))
> +		return;
> +
> +	pmu_reset();
> +
> +	write_regn(pmevtyper, 0, events[0] | PMEVTYPER_EXCLUDE_EL0);
> +	write_regn(pmevtyper, 1, events[1] | PMEVTYPER_EXCLUDE_EL0);
> +	/* enable counters #0 and #1 */
> +	write_sysreg_s(0x3, PMCNTENSET_EL0);
> +	/* preset counter #0 at 0xFFFFFFF0 */
> +	write_regn(pmevcntr, 0, 0xFFFFFFF0);
> +
> +	precise_instrs_loop(22, pmu.pmcr_ro | PMU_PMCR_E);
> +
> +	report(read_regn(pmevcntr, 1) == 1, "CHAIN counter #1 incremented");
> +	report(!read_sysreg(pmovsclr_el0), "check no overflow is recorded");
> +
> +	/* test 64b overflow */
> +
> +	pmu_reset();
> +	write_sysreg_s(0x3, PMCNTENSET_EL0);
> +
> +	write_regn(pmevcntr, 0, 0xFFFFFFF0);
> +	write_regn(pmevcntr, 1, 0x1);
> +	precise_instrs_loop(22, pmu.pmcr_ro | PMU_PMCR_E);
> +	report_info("overflow reg = 0x%lx", read_sysreg(pmovsclr_el0));
> +	report(read_regn(pmevcntr, 1) == 2, "CHAIN counter #1 incremented");
> +	report(!read_sysreg(pmovsclr_el0), "check no overflow is recorded");
> +
> +	write_regn(pmevcntr, 0, 0xFFFFFFF0);
> +	write_regn(pmevcntr, 1, 0xFFFFFFFF);
> +
> +	precise_instrs_loop(22, pmu.pmcr_ro | PMU_PMCR_E);
> +	report_info("overflow reg = 0x%lx", read_sysreg(pmovsclr_el0));
> +	report(!read_regn(pmevcntr, 1), "CHAIN counter #1 wrapped");
> +	report(read_sysreg(pmovsclr_el0) == 0x2,
> +		"check no overflow is recorded");
> +}
> +
> +static void test_chained_sw_incr(void)
> +{
> +	uint32_t events[] = { 0x0 /* SW_INCR */, 0x0 /* SW_INCR */};
> +	int i;
> +
> +	if (!satisfy_prerequisites(events, ARRAY_SIZE(events)))
> +		return;
> +
> +	pmu_reset();
> +
> +	write_regn(pmevtyper, 0, events[0] | PMEVTYPER_EXCLUDE_EL0);
> +	write_regn(pmevtyper, 1, events[1] | PMEVTYPER_EXCLUDE_EL0);
> +	/* enable counters #0 and #1 */
> +	write_sysreg_s(0x3, PMCNTENSET_EL0);
> +
> +	/* preset counter #0 at 0xFFFFFFF0 */
> +	write_regn(pmevcntr, 0, 0xFFFFFFF0);
> +
> +	for (i = 0; i < 100; i++)
> +		write_sysreg(0x1, pmswinc_el0);
> +
> +	report_info("SW_INCR counter #0 has value %ld", read_regn(pmevcntr, 0));
> +	report(read_regn(pmevcntr, 0) == 0xFFFFFFF0,
> +		"PWSYNC does not increment if PMCR.E is unset");
> +
> +	pmu_reset();
> +
> +	write_regn(pmevcntr, 0, 0xFFFFFFF0);
> +	write_sysreg_s(0x3, PMCNTENSET_EL0);
> +	set_pmcr(pmu.pmcr_ro | PMU_PMCR_E);
> +
> +	for (i = 0; i < 100; i++)
> +		write_sysreg(0x3, pmswinc_el0);
> +
> +	report(read_regn(pmevcntr, 0)  == 84, "counter #1 after + 100 SW_INCR");
> +	report(read_regn(pmevcntr, 1)  == 100,
> +		"counter #0 after + 100 SW_INCR");
> +	report_info("counter values after 100 SW_INCR #0=%ld #1=%ld",
> +		    read_regn(pmevcntr, 0), read_regn(pmevcntr, 1));
> +	report(read_sysreg(pmovsclr_el0) == 0x1,
> +		"overflow reg after 100 SW_INCR");
> +
> +	/* 64b SW_INCR */
> +	pmu_reset();
> +
> +	events[1] = 0x1E /* CHAIN */;
> +	write_regn(pmevtyper, 1, events[1] | PMEVTYPER_EXCLUDE_EL0);
> +	write_regn(pmevcntr, 0, 0xFFFFFFF0);
> +	write_sysreg_s(0x3, PMCNTENSET_EL0);
> +	set_pmcr(pmu.pmcr_ro | PMU_PMCR_E);
> +	for (i = 0; i < 100; i++)
> +		write_sysreg(0x3, pmswinc_el0);
> +
> +	report(!read_sysreg(pmovsclr_el0) && (read_regn(pmevcntr, 1) == 1),
> +		"overflow reg after 100 SW_INCR/CHAIN");
> +	report_info("overflow=0x%lx, #0=%ld #1=%ld", read_sysreg(pmovsclr_el0),
> +		    read_regn(pmevcntr, 0), read_regn(pmevcntr, 1));
> +
> +	/* 64b SW_INCR and overflow on CHAIN counter*/
> +	pmu_reset();
> +
> +	write_regn(pmevtyper, 1, events[1] | PMEVTYPER_EXCLUDE_EL0);
> +	write_regn(pmevcntr, 0, 0xFFFFFFF0);
> +	write_regn(pmevcntr, 1, 0xFFFFFFFF);
> +	write_sysreg_s(0x3, PMCNTENSET_EL0);
> +	set_pmcr(pmu.pmcr_ro | PMU_PMCR_E);
> +	for (i = 0; i < 100; i++)
> +		write_sysreg(0x3, pmswinc_el0);
> +
> +	report((read_sysreg(pmovsclr_el0) == 0x2) &&
> +		(read_regn(pmevcntr, 1) == 0) &&
> +		(read_regn(pmevcntr, 0) == 84),
> +		"overflow reg after 100 SW_INCR/CHAIN");
> +	report_info("overflow=0x%lx, #0=%ld #1=%ld", read_sysreg(pmovsclr_el0),
> +		    read_regn(pmevcntr, 0), read_regn(pmevcntr, 1));
> +}
> +
>  #endif
>  
>  /*
> @@ -657,6 +779,12 @@ int main(int argc, char *argv[])
>  	} else if (strcmp(argv[1], "mem-access") == 0) {
>  		report_prefix_push(argv[1]);
>  		test_mem_access();
> +	} else if (strcmp(argv[1], "chained-counters") == 0) {
> +		report_prefix_push(argv[1]);
> +		test_chained_counters();
> +	} else if (strcmp(argv[1], "chained-sw-incr") == 0) {
> +		report_prefix_push(argv[1]);
> +		test_chained_sw_incr();
>  	} else {
>  		report_abort("Unknown sub-test '%s'", argv[1]);
>  	}
> diff --git a/arm/unittests.cfg b/arm/unittests.cfg
> index 7a59403..1bd4319 100644
> --- a/arm/unittests.cfg
> +++ b/arm/unittests.cfg
> @@ -90,6 +90,18 @@ groups = pmu
>  arch = arm64
>  extra_params = -append 'mem-access'
>  
> +[pmu-chained-counters]
> +file = pmu.flat
> +groups = pmu
> +arch = arm64
> +extra_params = -append 'chained-counters'
> +
> +[pmu-chained-sw-incr]
> +file = pmu.flat
> +groups = pmu
> +arch = arm64
> +extra_params = -append 'chained-sw-incr'
> +
>  # Test PMU support (TCG) with -icount IPC=1
>  #[pmu-tcg-icount-1]
>  #file = pmu.flat
> -- 
> 2.20.1
> 
> 

Same comments as previous patch

Thanks,
drew

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[kvm-unit-tests PATCH v2 7/9] arm: pmu: test 32-bit <-> 64-bit transitions

[Eric Auger]

Test configurations where we transit from 32b to 64b
counters and conversely. Also tests configuration where
chain counters are configured but only one counter is
enabled.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 arm/pmu.c         | 136 ++++++++++++++++++++++++++++++++++++++++++++++
 arm/unittests.cfg |   6 ++
 2 files changed, 142 insertions(+)

diff --git a/arm/pmu.c b/arm/pmu.c
index 538fbeb..fa77ab3 100644
--- a/arm/pmu.c
+++ b/arm/pmu.c
@@ -115,6 +115,7 @@ static void test_basic_event_count(void) {}
 static void test_mem_access(void) {}
 static void test_chained_counters(void) {}
 static void test_chained_sw_incr(void) {}
+static void test_chain_promotion(void) {}
 
 #elif defined(__aarch64__)
 #define ID_AA64DFR0_PERFMON_SHIFT 8
@@ -580,6 +581,138 @@ static void test_chained_sw_incr(void)
 		    read_regn(pmevcntr, 0), read_regn(pmevcntr, 1));
 }
 
+static void test_chain_promotion(void)
+{
+	uint32_t events[] = { 0x13 /* MEM_ACCESS */, 0x1E /* CHAIN */};
+	void *addr = malloc(PAGE_SIZE);
+
+	if (!satisfy_prerequisites(events, ARRAY_SIZE(events)))
+		return;
+
+	/* Only enable CHAIN counter */
+	pmu_reset();
+	write_regn(pmevtyper, 0, events[0] | PMEVTYPER_EXCLUDE_EL0);
+	write_regn(pmevtyper, 1, events[1] | PMEVTYPER_EXCLUDE_EL0);
+	write_sysreg_s(0x2, PMCNTENSET_EL0);
+	isb();
+
+	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E);
+	report(!read_regn(pmevcntr, 0),
+		"chain counter not counting if even counter is disabled");
+
+	/* Only enable even counter */
+	pmu_reset();
+	write_regn(pmevcntr, 0, 0xFFFFFFF0);
+	write_sysreg_s(0x1, PMCNTENSET_EL0);
+	isb();
+
+	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E);
+	report(!read_regn(pmevcntr, 1) && (read_sysreg(pmovsclr_el0) == 0x1),
+		"odd counter did not increment on overflow if disabled");
+	report_info("MEM_ACCESS counter #0 has value %ld",
+		    read_regn(pmevcntr, 0));
+	report_info("CHAIN counter #1 has value %ld",
+		    read_regn(pmevcntr, 1));
+	report_info("overflow counter %ld", read_sysreg(pmovsclr_el0));
+
+	/* start at 0xFFFFFFDC, +20 with CHAIN enabled, +20 with CHAIN disabled */
+	pmu_reset();
+	write_sysreg_s(0x3, PMCNTENSET_EL0);
+	write_regn(pmevcntr, 0, 0xFFFFFFDC);
+	isb();
+
+	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E);
+	report_info("MEM_ACCESS counter #0 has value 0x%lx",
+		    read_regn(pmevcntr, 0));
+
+	/* disable the CHAIN event */
+	write_sysreg_s(0x2, PMCNTENCLR_EL0);
+	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E);
+	report_info("MEM_ACCESS counter #0 has value 0x%lx",
+		    read_regn(pmevcntr, 0));
+	report(read_sysreg(pmovsclr_el0) == 0x1,
+		"should have triggered an overflow on #0");
+	report(!read_regn(pmevcntr, 1),
+		"CHAIN counter #1 shouldn't have incremented");
+
+	/* start at 0xFFFFFFDC, +20 with CHAIN disabled, +20 with CHAIN enabled */
+
+	pmu_reset();
+	write_sysreg_s(0x1, PMCNTENSET_EL0);
+	write_regn(pmevcntr, 0, 0xFFFFFFDC);
+	isb();
+	report_info("counter #0 = 0x%lx, counter #1 = 0x%lx overflow=0x%lx",
+		    read_regn(pmevcntr, 0), read_regn(pmevcntr, 1),
+		    read_sysreg(pmovsclr_el0));
+
+	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E);
+	report_info("MEM_ACCESS counter #0 has value 0x%lx",
+		    read_regn(pmevcntr, 0));
+
+	/* enable the CHAIN event */
+	write_sysreg_s(0x3, PMCNTENSET_EL0);
+	isb();
+	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E);
+	report_info("MEM_ACCESS counter #0 has value 0x%lx",
+		    read_regn(pmevcntr, 0));
+
+	report((read_regn(pmevcntr, 1) == 1) && !read_sysreg(pmovsclr_el0),
+		"CHAIN counter #1 should have incremented and no overflow expected");
+
+	report_info("CHAIN counter #1 = 0x%lx, overflow=0x%lx",
+		read_regn(pmevcntr, 1), read_sysreg(pmovsclr_el0));
+
+	/* start as MEM_ACCESS/CPU_CYCLES and move to CHAIN/MEM_ACCESS */
+	pmu_reset();
+	write_regn(pmevtyper, 0, 0x13 /* MEM_ACCESS */ | PMEVTYPER_EXCLUDE_EL0);
+	write_regn(pmevtyper, 1, 0x11 /* CPU_CYCLES */ | PMEVTYPER_EXCLUDE_EL0);
+	write_sysreg_s(0x3, PMCNTENSET_EL0);
+	write_regn(pmevcntr, 0, 0xFFFFFFDC);
+	isb();
+
+	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E);
+	report_info("MEM_ACCESS counter #0 has value 0x%lx",
+		    read_regn(pmevcntr, 0));
+
+	/* 0 becomes CHAINED */
+	write_sysreg_s(0x0, PMCNTENSET_EL0);
+	write_regn(pmevtyper, 1, 0x1E /* CHAIN */ | PMEVTYPER_EXCLUDE_EL0);
+	write_sysreg_s(0x3, PMCNTENSET_EL0);
+	write_regn(pmevcntr, 1, 0x0);
+
+	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E);
+	report_info("MEM_ACCESS counter #0 has value 0x%lx",
+		    read_regn(pmevcntr, 0));
+
+	report((read_regn(pmevcntr, 1) == 1) && !read_sysreg(pmovsclr_el0),
+		"CHAIN counter #1 should have incremented and no overflow expected");
+
+	report_info("CHAIN counter #1 = 0x%lx, overflow=0x%lx",
+		read_regn(pmevcntr, 1), read_sysreg(pmovsclr_el0));
+
+	/* start as CHAIN/MEM_ACCESS and move to MEM_ACCESS/CPU_CYCLES */
+	pmu_reset();
+	write_regn(pmevtyper, 0, 0x13 /* MEM_ACCESS */ | PMEVTYPER_EXCLUDE_EL0);
+	write_regn(pmevtyper, 1, 0x1E /* CHAIN */ | PMEVTYPER_EXCLUDE_EL0);
+	write_regn(pmevcntr, 0, 0xFFFFFFDC);
+	write_sysreg_s(0x3, PMCNTENSET_EL0);
+
+	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E);
+	report_info("counter #0=0x%lx, counter #1=0x%lx",
+			read_regn(pmevcntr, 0), read_regn(pmevcntr, 1));
+
+	write_sysreg_s(0x0, PMCNTENSET_EL0);
+	write_regn(pmevtyper, 1, 0x11 /* CPU_CYCLES */ | PMEVTYPER_EXCLUDE_EL0);
+	write_sysreg_s(0x3, PMCNTENSET_EL0);
+
+	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E);
+	report(read_sysreg(pmovsclr_el0) == 1,
+		"overflow is expected on counter 0");
+	report_info("counter #0=0x%lx, counter #1=0x%lx overflow=0x%lx",
+			read_regn(pmevcntr, 0), read_regn(pmevcntr, 1),
+			read_sysreg(pmovsclr_el0));
+}
+
 #endif
 
 /*
@@ -785,6 +918,9 @@ int main(int argc, char *argv[])
 	} else if (strcmp(argv[1], "chained-sw-incr") == 0) {
 		report_prefix_push(argv[1]);
 		test_chained_sw_incr();
+	} else if (strcmp(argv[1], "chain-promotion") == 0) {
+		report_prefix_push(argv[1]);
+		test_chain_promotion();
 	} else {
 		report_abort("Unknown sub-test '%s'", argv[1]);
 	}
diff --git a/arm/unittests.cfg b/arm/unittests.cfg
index 1bd4319..eb6e87e 100644
--- a/arm/unittests.cfg
+++ b/arm/unittests.cfg
@@ -102,6 +102,12 @@ groups = pmu
 arch = arm64
 extra_params = -append 'chained-sw-incr'
 
+[pmu-chain-promotion]
+file = pmu.flat
+groups = pmu
+arch = arm64
+extra_params = -append 'chain-promotion'
+
 # Test PMU support (TCG) with -icount IPC=1
 #[pmu-tcg-icount-1]
 #file = pmu.flat
-- 
2.20.1

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [kvm-unit-tests PATCH v2 7/9] arm: pmu: test 32-bit <-> 64-bit transitions

[Andrew Jones]

On Thu, Jan 30, 2020 at 12:25:08PM +0100, Eric Auger wrote:
> Test configurations where we transit from 32b to 64b
> counters and conversely. Also tests configuration where
> chain counters are configured but only one counter is
> enabled.
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> ---
>  arm/pmu.c         | 136 ++++++++++++++++++++++++++++++++++++++++++++++
>  arm/unittests.cfg |   6 ++
>  2 files changed, 142 insertions(+)
> 
> diff --git a/arm/pmu.c b/arm/pmu.c
> index 538fbeb..fa77ab3 100644
> --- a/arm/pmu.c
> +++ b/arm/pmu.c
> @@ -115,6 +115,7 @@ static void test_basic_event_count(void) {}
>  static void test_mem_access(void) {}
>  static void test_chained_counters(void) {}
>  static void test_chained_sw_incr(void) {}
> +static void test_chain_promotion(void) {}
>  
>  #elif defined(__aarch64__)
>  #define ID_AA64DFR0_PERFMON_SHIFT 8
> @@ -580,6 +581,138 @@ static void test_chained_sw_incr(void)
>  		    read_regn(pmevcntr, 0), read_regn(pmevcntr, 1));
>  }
>  
> +static void test_chain_promotion(void)
> +{
> +	uint32_t events[] = { 0x13 /* MEM_ACCESS */, 0x1E /* CHAIN */};
> +	void *addr = malloc(PAGE_SIZE);
> +
> +	if (!satisfy_prerequisites(events, ARRAY_SIZE(events)))
> +		return;
> +
> +	/* Only enable CHAIN counter */
> +	pmu_reset();
> +	write_regn(pmevtyper, 0, events[0] | PMEVTYPER_EXCLUDE_EL0);
> +	write_regn(pmevtyper, 1, events[1] | PMEVTYPER_EXCLUDE_EL0);
> +	write_sysreg_s(0x2, PMCNTENSET_EL0);
> +	isb();
> +
> +	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E);
> +	report(!read_regn(pmevcntr, 0),
> +		"chain counter not counting if even counter is disabled");
> +
> +	/* Only enable even counter */
> +	pmu_reset();
> +	write_regn(pmevcntr, 0, 0xFFFFFFF0);
> +	write_sysreg_s(0x1, PMCNTENSET_EL0);
> +	isb();
> +
> +	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E);
> +	report(!read_regn(pmevcntr, 1) && (read_sysreg(pmovsclr_el0) == 0x1),
> +		"odd counter did not increment on overflow if disabled");
> +	report_info("MEM_ACCESS counter #0 has value %ld",
> +		    read_regn(pmevcntr, 0));
> +	report_info("CHAIN counter #1 has value %ld",
> +		    read_regn(pmevcntr, 1));
> +	report_info("overflow counter %ld", read_sysreg(pmovsclr_el0));
> +
> +	/* start at 0xFFFFFFDC, +20 with CHAIN enabled, +20 with CHAIN disabled */
> +	pmu_reset();
> +	write_sysreg_s(0x3, PMCNTENSET_EL0);
> +	write_regn(pmevcntr, 0, 0xFFFFFFDC);
> +	isb();
> +
> +	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E);
> +	report_info("MEM_ACCESS counter #0 has value 0x%lx",
> +		    read_regn(pmevcntr, 0));
> +
> +	/* disable the CHAIN event */
> +	write_sysreg_s(0x2, PMCNTENCLR_EL0);
> +	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E);
> +	report_info("MEM_ACCESS counter #0 has value 0x%lx",
> +		    read_regn(pmevcntr, 0));
> +	report(read_sysreg(pmovsclr_el0) == 0x1,
> +		"should have triggered an overflow on #0");
> +	report(!read_regn(pmevcntr, 1),
> +		"CHAIN counter #1 shouldn't have incremented");
> +
> +	/* start at 0xFFFFFFDC, +20 with CHAIN disabled, +20 with CHAIN enabled */
> +
> +	pmu_reset();
> +	write_sysreg_s(0x1, PMCNTENSET_EL0);
> +	write_regn(pmevcntr, 0, 0xFFFFFFDC);
> +	isb();
> +	report_info("counter #0 = 0x%lx, counter #1 = 0x%lx overflow=0x%lx",
> +		    read_regn(pmevcntr, 0), read_regn(pmevcntr, 1),
> +		    read_sysreg(pmovsclr_el0));
> +
> +	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E);
> +	report_info("MEM_ACCESS counter #0 has value 0x%lx",
> +		    read_regn(pmevcntr, 0));
> +
> +	/* enable the CHAIN event */
> +	write_sysreg_s(0x3, PMCNTENSET_EL0);
> +	isb();
> +	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E);
> +	report_info("MEM_ACCESS counter #0 has value 0x%lx",
> +		    read_regn(pmevcntr, 0));
> +
> +	report((read_regn(pmevcntr, 1) == 1) && !read_sysreg(pmovsclr_el0),
> +		"CHAIN counter #1 should have incremented and no overflow expected");
> +
> +	report_info("CHAIN counter #1 = 0x%lx, overflow=0x%lx",
> +		read_regn(pmevcntr, 1), read_sysreg(pmovsclr_el0));
> +
> +	/* start as MEM_ACCESS/CPU_CYCLES and move to CHAIN/MEM_ACCESS */
> +	pmu_reset();
> +	write_regn(pmevtyper, 0, 0x13 /* MEM_ACCESS */ | PMEVTYPER_EXCLUDE_EL0);
> +	write_regn(pmevtyper, 1, 0x11 /* CPU_CYCLES */ | PMEVTYPER_EXCLUDE_EL0);
> +	write_sysreg_s(0x3, PMCNTENSET_EL0);
> +	write_regn(pmevcntr, 0, 0xFFFFFFDC);
> +	isb();
> +
> +	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E);
> +	report_info("MEM_ACCESS counter #0 has value 0x%lx",
> +		    read_regn(pmevcntr, 0));
> +
> +	/* 0 becomes CHAINED */
> +	write_sysreg_s(0x0, PMCNTENSET_EL0);
> +	write_regn(pmevtyper, 1, 0x1E /* CHAIN */ | PMEVTYPER_EXCLUDE_EL0);
> +	write_sysreg_s(0x3, PMCNTENSET_EL0);
> +	write_regn(pmevcntr, 1, 0x0);
> +
> +	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E);
> +	report_info("MEM_ACCESS counter #0 has value 0x%lx",
> +		    read_regn(pmevcntr, 0));
> +
> +	report((read_regn(pmevcntr, 1) == 1) && !read_sysreg(pmovsclr_el0),
> +		"CHAIN counter #1 should have incremented and no overflow expected");
> +
> +	report_info("CHAIN counter #1 = 0x%lx, overflow=0x%lx",
> +		read_regn(pmevcntr, 1), read_sysreg(pmovsclr_el0));
> +
> +	/* start as CHAIN/MEM_ACCESS and move to MEM_ACCESS/CPU_CYCLES */
> +	pmu_reset();
> +	write_regn(pmevtyper, 0, 0x13 /* MEM_ACCESS */ | PMEVTYPER_EXCLUDE_EL0);
> +	write_regn(pmevtyper, 1, 0x1E /* CHAIN */ | PMEVTYPER_EXCLUDE_EL0);
> +	write_regn(pmevcntr, 0, 0xFFFFFFDC);
> +	write_sysreg_s(0x3, PMCNTENSET_EL0);
> +
> +	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E);
> +	report_info("counter #0=0x%lx, counter #1=0x%lx",
> +			read_regn(pmevcntr, 0), read_regn(pmevcntr, 1));
> +
> +	write_sysreg_s(0x0, PMCNTENSET_EL0);
> +	write_regn(pmevtyper, 1, 0x11 /* CPU_CYCLES */ | PMEVTYPER_EXCLUDE_EL0);
> +	write_sysreg_s(0x3, PMCNTENSET_EL0);
> +
> +	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E);
> +	report(read_sysreg(pmovsclr_el0) == 1,
> +		"overflow is expected on counter 0");
> +	report_info("counter #0=0x%lx, counter #1=0x%lx overflow=0x%lx",
> +			read_regn(pmevcntr, 0), read_regn(pmevcntr, 1),
> +			read_sysreg(pmovsclr_el0));
> +}
> +
>  #endif
>  
>  /*
> @@ -785,6 +918,9 @@ int main(int argc, char *argv[])
>  	} else if (strcmp(argv[1], "chained-sw-incr") == 0) {
>  		report_prefix_push(argv[1]);
>  		test_chained_sw_incr();
> +	} else if (strcmp(argv[1], "chain-promotion") == 0) {
> +		report_prefix_push(argv[1]);
> +		test_chain_promotion();
>  	} else {
>  		report_abort("Unknown sub-test '%s'", argv[1]);
>  	}
> diff --git a/arm/unittests.cfg b/arm/unittests.cfg
> index 1bd4319..eb6e87e 100644
> --- a/arm/unittests.cfg
> +++ b/arm/unittests.cfg
> @@ -102,6 +102,12 @@ groups = pmu
>  arch = arm64
>  extra_params = -append 'chained-sw-incr'
>  
> +[pmu-chain-promotion]
> +file = pmu.flat
> +groups = pmu
> +arch = arm64
> +extra_params = -append 'chain-promotion'
> +
>  # Test PMU support (TCG) with -icount IPC=1
>  #[pmu-tcg-icount-1]
>  #file = pmu.flat
> -- 
> 2.20.1
> 
> 

same comments as previous patches

Thanks,
drew

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[kvm-unit-tests PATCH v2 8/9] arm: gic: Provide per-IRQ helper functions

[Eric Auger]

From: Andre Przywara <andre.przywara@arm.com>

A common theme when accessing per-IRQ parameters in the GIC distributor
is to set fields of a certain bit width in a range of MMIO registers.
Examples are the enabled status (one bit per IRQ), the level/edge
configuration (2 bits per IRQ) or the priority (8 bits per IRQ).

Add a generic helper function which is able to mask and set the
respective number of bits, given the IRQ number and the MMIO offset.
Provide wrappers using this function to easily allow configuring an IRQ.

For now assume that private IRQ numbers always refer to the current CPU.
In a GICv2 accessing the "other" private IRQs is not easily doable (the
registers are banked per CPU on the same MMIO address), so we impose the
same limitation on GICv3, even though those registers are not banked
there anymore.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>

---

initialize reg
---
 lib/arm/asm/gic-v3.h |  2 +
 lib/arm/asm/gic.h    |  9 +++++
 lib/arm/gic.c        | 90 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 101 insertions(+)

diff --git a/lib/arm/asm/gic-v3.h b/lib/arm/asm/gic-v3.h
index 347be2f..4a445a5 100644
--- a/lib/arm/asm/gic-v3.h
+++ b/lib/arm/asm/gic-v3.h
@@ -23,6 +23,8 @@
 #define GICD_CTLR_ENABLE_G1A		(1U << 1)
 #define GICD_CTLR_ENABLE_G1		(1U << 0)
 
+#define GICD_IROUTER			0x6000
+
 /* Re-Distributor registers, offsets from RD_base */
 #define GICR_TYPER			0x0008
 
diff --git a/lib/arm/asm/gic.h b/lib/arm/asm/gic.h
index 1fc10a0..21cdb58 100644
--- a/lib/arm/asm/gic.h
+++ b/lib/arm/asm/gic.h
@@ -15,6 +15,7 @@
 #define GICD_IIDR			0x0008
 #define GICD_IGROUPR			0x0080
 #define GICD_ISENABLER			0x0100
+#define GICD_ICENABLER			0x0180
 #define GICD_ISPENDR			0x0200
 #define GICD_ICPENDR			0x0280
 #define GICD_ISACTIVER			0x0300
@@ -73,5 +74,13 @@ extern void gic_write_eoir(u32 irqstat);
 extern void gic_ipi_send_single(int irq, int cpu);
 extern void gic_ipi_send_mask(int irq, const cpumask_t *dest);
 
+void gic_set_irq_bit(int irq, int offset);
+void gic_enable_irq(int irq);
+void gic_disable_irq(int irq);
+void gic_set_irq_priority(int irq, u8 prio);
+void gic_set_irq_target(int irq, int cpu);
+void gic_set_irq_group(int irq, int group);
+int gic_get_irq_group(int irq);
+
 #endif /* !__ASSEMBLY__ */
 #endif /* _ASMARM_GIC_H_ */
diff --git a/lib/arm/gic.c b/lib/arm/gic.c
index 9430116..aa9cb86 100644
--- a/lib/arm/gic.c
+++ b/lib/arm/gic.c
@@ -146,3 +146,93 @@ void gic_ipi_send_mask(int irq, const cpumask_t *dest)
 	assert(gic_common_ops && gic_common_ops->ipi_send_mask);
 	gic_common_ops->ipi_send_mask(irq, dest);
 }
+
+enum gic_bit_access {
+	ACCESS_READ,
+	ACCESS_SET,
+	ACCESS_RMW
+};
+
+static u8 gic_masked_irq_bits(int irq, int offset, int bits, u8 value,
+			      enum gic_bit_access access)
+{
+	void *base;
+	int split = 32 / bits;
+	int shift = (irq % split) * bits;
+	u32 reg = 0, mask = ((1U << bits) - 1) << shift;
+
+	switch (gic_version()) {
+	case 2:
+		base = gicv2_dist_base();
+		break;
+	case 3:
+		if (irq < 32)
+			base = gicv3_sgi_base();
+		else
+			base = gicv3_dist_base();
+		break;
+	default:
+		return 0;
+	}
+	base += offset + (irq / split) * 4;
+
+	switch (access) {
+	case ACCESS_READ:
+		return (readl(base) & mask) >> shift;
+	case ACCESS_SET:
+		reg = 0;
+		break;
+	case ACCESS_RMW:
+		reg = readl(base) & ~mask;
+		break;
+	}
+
+	writel(reg | ((u32)value << shift), base);
+
+	return 0;
+}
+
+void gic_set_irq_bit(int irq, int offset)
+{
+	gic_masked_irq_bits(irq, offset, 1, 1, ACCESS_SET);
+}
+
+void gic_enable_irq(int irq)
+{
+	gic_set_irq_bit(irq, GICD_ISENABLER);
+}
+
+void gic_disable_irq(int irq)
+{
+	gic_set_irq_bit(irq, GICD_ICENABLER);
+}
+
+void gic_set_irq_priority(int irq, u8 prio)
+{
+	gic_masked_irq_bits(irq, GICD_IPRIORITYR, 8, prio, ACCESS_RMW);
+}
+
+void gic_set_irq_target(int irq, int cpu)
+{
+	if (irq < 32)
+		return;
+
+	if (gic_version() == 2) {
+		gic_masked_irq_bits(irq, GICD_ITARGETSR, 8, 1U << cpu,
+				    ACCESS_RMW);
+
+		return;
+	}
+
+	writeq(cpus[cpu], gicv3_dist_base() + GICD_IROUTER + irq * 8);
+}
+
+void gic_set_irq_group(int irq, int group)
+{
+	gic_masked_irq_bits(irq, GICD_IGROUPR, 1, group, ACCESS_RMW);
+}
+
+int gic_get_irq_group(int irq)
+{
+	return gic_masked_irq_bits(irq, GICD_IGROUPR, 1, 0, ACCESS_READ);
+}
-- 
2.20.1

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [kvm-unit-tests PATCH v2 8/9] arm: gic: Provide per-IRQ helper functions

[Andrew Jones]

On Thu, Jan 30, 2020 at 12:25:09PM +0100, Eric Auger wrote:
> From: Andre Przywara <andre.przywara@arm.com>
> 
> A common theme when accessing per-IRQ parameters in the GIC distributor
> is to set fields of a certain bit width in a range of MMIO registers.
> Examples are the enabled status (one bit per IRQ), the level/edge
> configuration (2 bits per IRQ) or the priority (8 bits per IRQ).
> 
> Add a generic helper function which is able to mask and set the
> respective number of bits, given the IRQ number and the MMIO offset.
> Provide wrappers using this function to easily allow configuring an IRQ.
> 
> For now assume that private IRQ numbers always refer to the current CPU.
> In a GICv2 accessing the "other" private IRQs is not easily doable (the
> registers are banked per CPU on the same MMIO address), so we impose the
> same limitation on GICv3, even though those registers are not banked
> there anymore.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>

Missing Eric's s-b.

> 
> ---
> 
> initialize reg
> ---
>  lib/arm/asm/gic-v3.h |  2 +
>  lib/arm/asm/gic.h    |  9 +++++
>  lib/arm/gic.c        | 90 ++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 101 insertions(+)
> 
> diff --git a/lib/arm/asm/gic-v3.h b/lib/arm/asm/gic-v3.h
> index 347be2f..4a445a5 100644
> --- a/lib/arm/asm/gic-v3.h
> +++ b/lib/arm/asm/gic-v3.h
> @@ -23,6 +23,8 @@
>  #define GICD_CTLR_ENABLE_G1A		(1U << 1)
>  #define GICD_CTLR_ENABLE_G1		(1U << 0)
>  
> +#define GICD_IROUTER			0x6000
> +
>  /* Re-Distributor registers, offsets from RD_base */
>  #define GICR_TYPER			0x0008
>  
> diff --git a/lib/arm/asm/gic.h b/lib/arm/asm/gic.h
> index 1fc10a0..21cdb58 100644
> --- a/lib/arm/asm/gic.h
> +++ b/lib/arm/asm/gic.h
> @@ -15,6 +15,7 @@
>  #define GICD_IIDR			0x0008
>  #define GICD_IGROUPR			0x0080
>  #define GICD_ISENABLER			0x0100
> +#define GICD_ICENABLER			0x0180
>  #define GICD_ISPENDR			0x0200
>  #define GICD_ICPENDR			0x0280
>  #define GICD_ISACTIVER			0x0300
> @@ -73,5 +74,13 @@ extern void gic_write_eoir(u32 irqstat);
>  extern void gic_ipi_send_single(int irq, int cpu);
>  extern void gic_ipi_send_mask(int irq, const cpumask_t *dest);
>  
> +void gic_set_irq_bit(int irq, int offset);
> +void gic_enable_irq(int irq);
> +void gic_disable_irq(int irq);
> +void gic_set_irq_priority(int irq, u8 prio);
> +void gic_set_irq_target(int irq, int cpu);
> +void gic_set_irq_group(int irq, int group);
> +int gic_get_irq_group(int irq);
> +
>  #endif /* !__ASSEMBLY__ */
>  #endif /* _ASMARM_GIC_H_ */
> diff --git a/lib/arm/gic.c b/lib/arm/gic.c
> index 9430116..aa9cb86 100644
> --- a/lib/arm/gic.c
> +++ b/lib/arm/gic.c
> @@ -146,3 +146,93 @@ void gic_ipi_send_mask(int irq, const cpumask_t *dest)
>  	assert(gic_common_ops && gic_common_ops->ipi_send_mask);
>  	gic_common_ops->ipi_send_mask(irq, dest);
>  }
> +
> +enum gic_bit_access {
> +	ACCESS_READ,
> +	ACCESS_SET,
> +	ACCESS_RMW
> +};
> +
> +static u8 gic_masked_irq_bits(int irq, int offset, int bits, u8 value,
> +			      enum gic_bit_access access)
> +{
> +	void *base;
> +	int split = 32 / bits;
> +	int shift = (irq % split) * bits;
> +	u32 reg = 0, mask = ((1U << bits) - 1) << shift;
> +
> +	switch (gic_version()) {
> +	case 2:
> +		base = gicv2_dist_base();
> +		break;
> +	case 3:
> +		if (irq < 32)
> +			base = gicv3_sgi_base();
> +		else
> +			base = gicv3_dist_base();
> +		break;
> +	default:
> +		return 0;
> +	}
> +	base += offset + (irq / split) * 4;
> +
> +	switch (access) {
> +	case ACCESS_READ:
> +		return (readl(base) & mask) >> shift;
> +	case ACCESS_SET:
> +		reg = 0;
> +		break;
> +	case ACCESS_RMW:
> +		reg = readl(base) & ~mask;
> +		break;
> +	}
> +
> +	writel(reg | ((u32)value << shift), base);
> +
> +	return 0;
> +}
> +
> +void gic_set_irq_bit(int irq, int offset)
> +{
> +	gic_masked_irq_bits(irq, offset, 1, 1, ACCESS_SET);
> +}
> +
> +void gic_enable_irq(int irq)
> +{
> +	gic_set_irq_bit(irq, GICD_ISENABLER);
> +}
> +
> +void gic_disable_irq(int irq)
> +{
> +	gic_set_irq_bit(irq, GICD_ICENABLER);
> +}
> +
> +void gic_set_irq_priority(int irq, u8 prio)
> +{
> +	gic_masked_irq_bits(irq, GICD_IPRIORITYR, 8, prio, ACCESS_RMW);
> +}
> +
> +void gic_set_irq_target(int irq, int cpu)
> +{
> +	if (irq < 32)
> +		return;
> +
> +	if (gic_version() == 2) {
> +		gic_masked_irq_bits(irq, GICD_ITARGETSR, 8, 1U << cpu,
> +				    ACCESS_RMW);
> +
> +		return;
> +	}
> +
> +	writeq(cpus[cpu], gicv3_dist_base() + GICD_IROUTER + irq * 8);
> +}
> +
> +void gic_set_irq_group(int irq, int group)
> +{
> +	gic_masked_irq_bits(irq, GICD_IGROUPR, 1, group, ACCESS_RMW);
> +}
> +
> +int gic_get_irq_group(int irq)
> +{
> +	return gic_masked_irq_bits(irq, GICD_IGROUPR, 1, 0, ACCESS_READ);
> +}
> -- 
> 2.20.1
> 
>

Looks good to me.

Thanks,
drew

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [kvm-unit-tests PATCH v2 8/9] arm: gic: Provide per-IRQ helper functions

[Alexandru Elisei]

Hi,

I'm going to reiterate the comments I posted on this exact same patch sent by
Andre [1], and add a few new ones.

[1] https://www.spinics.net/lists/arm-kernel/msg767690.html

On 3/5/20 9:55 AM, Andrew Jones wrote:
> On Thu, Jan 30, 2020 at 12:25:09PM +0100, Eric Auger wrote:
>> From: Andre Przywara <andre.przywara@arm.com>
>>
>> A common theme when accessing per-IRQ parameters in the GIC distributor
>> is to set fields of a certain bit width in a range of MMIO registers.
>> Examples are the enabled status (one bit per IRQ), the level/edge
>> configuration (2 bits per IRQ) or the priority (8 bits per IRQ).
>>
>> Add a generic helper function which is able to mask and set the
>> respective number of bits, given the IRQ number and the MMIO offset.
>> Provide wrappers using this function to easily allow configuring an IRQ.
>>
>> For now assume that private IRQ numbers always refer to the current CPU.
>> In a GICv2 accessing the "other" private IRQs is not easily doable (the
>> registers are banked per CPU on the same MMIO address), so we impose the
>> same limitation on GICv3, even though those registers are not banked
>> there anymore.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> Missing Eric's s-b.
>
>> ---
>>
>> initialize reg
>> ---
>>  lib/arm/asm/gic-v3.h |  2 +
>>  lib/arm/asm/gic.h    |  9 +++++
>>  lib/arm/gic.c        | 90 ++++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 101 insertions(+)
>>
>> diff --git a/lib/arm/asm/gic-v3.h b/lib/arm/asm/gic-v3.h
>> index 347be2f..4a445a5 100644
>> --- a/lib/arm/asm/gic-v3.h
>> +++ b/lib/arm/asm/gic-v3.h
>> @@ -23,6 +23,8 @@
>>  #define GICD_CTLR_ENABLE_G1A		(1U << 1)
>>  #define GICD_CTLR_ENABLE_G1		(1U << 0)
>>  
>> +#define GICD_IROUTER			0x6000
>> +
>>  /* Re-Distributor registers, offsets from RD_base */
>>  #define GICR_TYPER			0x0008
>>  
>> diff --git a/lib/arm/asm/gic.h b/lib/arm/asm/gic.h
>> index 1fc10a0..21cdb58 100644
>> --- a/lib/arm/asm/gic.h
>> +++ b/lib/arm/asm/gic.h
>> @@ -15,6 +15,7 @@
>>  #define GICD_IIDR			0x0008
>>  #define GICD_IGROUPR			0x0080
>>  #define GICD_ISENABLER			0x0100
>> +#define GICD_ICENABLER			0x0180
>>  #define GICD_ISPENDR			0x0200
>>  #define GICD_ICPENDR			0x0280
>>  #define GICD_ISACTIVER			0x0300
>> @@ -73,5 +74,13 @@ extern void gic_write_eoir(u32 irqstat);
>>  extern void gic_ipi_send_single(int irq, int cpu);
>>  extern void gic_ipi_send_mask(int irq, const cpumask_t *dest);
>>  
>> +void gic_set_irq_bit(int irq, int offset);
>> +void gic_enable_irq(int irq);
>> +void gic_disable_irq(int irq);
>> +void gic_set_irq_priority(int irq, u8 prio);
>> +void gic_set_irq_target(int irq, int cpu);
>> +void gic_set_irq_group(int irq, int group);
>> +int gic_get_irq_group(int irq);
>> +
>>  #endif /* !__ASSEMBLY__ */
>>  #endif /* _ASMARM_GIC_H_ */
>> diff --git a/lib/arm/gic.c b/lib/arm/gic.c
>> index 9430116..aa9cb86 100644
>> --- a/lib/arm/gic.c
>> +++ b/lib/arm/gic.c
>> @@ -146,3 +146,93 @@ void gic_ipi_send_mask(int irq, const cpumask_t *dest)
>>  	assert(gic_common_ops && gic_common_ops->ipi_send_mask);
>>  	gic_common_ops->ipi_send_mask(irq, dest);
>>  }
>> +
>> +enum gic_bit_access {
>> +	ACCESS_READ,
>> +	ACCESS_SET,
>> +	ACCESS_RMW
>> +};
>> +
>> +static u8 gic_masked_irq_bits(int irq, int offset, int bits, u8 value,
>> +			      enum gic_bit_access access)
>> +{
>> +	void *base;
>> +	int split = 32 / bits;
>> +	int shift = (irq % split) * bits;
>> +	u32 reg = 0, mask = ((1U << bits) - 1) << shift;
>> +
>> +	switch (gic_version()) {
>> +	case 2:
>> +		base = gicv2_dist_base();
>> +		break;
>> +	case 3:
>> +		if (irq < 32)
>> +			base = gicv3_sgi_base();
>> +		else
>> +			base = gicv3_dist_base();
>> +		break;
>> +	default:
>> +		return 0;
>> +	}
>> +	base += offset + (irq / split) * 4;
>> +
>> +	switch (access) {
>> +	case ACCESS_READ:
>> +		return (readl(base) & mask) >> shift;
>> +	case ACCESS_SET:
>> +		reg = 0;
>> +		break;
>> +	case ACCESS_RMW:
>> +		reg = readl(base) & ~mask;
>> +		break;
>> +	}
>> +
>> +	writel(reg | ((u32)value << shift), base);
>> +
>> +	return 0;
>> +}

This function looks a bit out of place:
- the function name has a verb in the past tense ('masked'), which makes me think
it should return a bool, but the function actually performs an access to a GIC
register.
- the return value is an u8, but it returns an u32 on a read, because readl
returns an u32.
- the semantics of the function and the return value change based on the access
parameter; worse yet, the return value on a write is completely ignored by the
callers and the value parameter is ignored on reads.

And a few new niggles:
- the accessor functions are hard to read: git_masked_irq_bits(irq, offset, 1, 1, ACCESS_SET). What the 1's represent is not obvious without reading the gic_masked_irq_bits functions.
- Andrew introduced a function to check the IRQ state at the GIC level [2]. The function is clear and nice to read because it has no indirection. How about we follow this pattern for all the accessors below and we remove the calls to gic_masked_irq_bits entirely? 

[2] https://www.spinics.net/lists/kvm/msg207073.html

Thanks,
Alex

>> +
>> +void gic_set_irq_bit(int irq, int offset)
>> +{
>> +	gic_masked_irq_bits(irq, offset, 1, 1, ACCESS_SET);
>> +}
>> +
>> +void gic_enable_irq(int irq)
>> +{
>> +	gic_set_irq_bit(irq, GICD_ISENABLER);
>> +}
>> +
>> +void gic_disable_irq(int irq)
>> +{
>> +	gic_set_irq_bit(irq, GICD_ICENABLER);
>> +}
>> +
>> +void gic_set_irq_priority(int irq, u8 prio)
>> +{
>> +	gic_masked_irq_bits(irq, GICD_IPRIORITYR, 8, prio, ACCESS_RMW);
>> +}
>> +
>> +void gic_set_irq_target(int irq, int cpu)
>> +{
>> +	if (irq < 32)
>> +		return;
>> +
>> +	if (gic_version() == 2) {
>> +		gic_masked_irq_bits(irq, GICD_ITARGETSR, 8, 1U << cpu,
>> +				    ACCESS_RMW);
>> +
>> +		return;
>> +	}
>> +
>> +	writeq(cpus[cpu], gicv3_dist_base() + GICD_IROUTER + irq * 8);
>> +}
>> +
>> +void gic_set_irq_group(int irq, int group)
>> +{
>> +	gic_masked_irq_bits(irq, GICD_IGROUPR, 1, group, ACCESS_RMW);
>> +}
>> +
>> +int gic_get_irq_group(int irq)
>> +{
>> +	return gic_masked_irq_bits(irq, GICD_IGROUPR, 1, 0, ACCESS_READ);
>> +}
>> -- 
>> 2.20.1
>>
>>
> Looks good to me.
>
> Thanks,
> drew
>
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[kvm-unit-tests PATCH v2 9/9] arm: pmu: Test overflow interrupts

[Eric Auger]

Test overflows for MEM_ACCESS and SW_INCR events. Also tests
overflows with 64-bit events.

Signed-off-by: Eric Auger <eric.auger@redhat.com>

---

v1 -> v2:
- inline setup_irq() code
---
 arm/pmu.c         | 137 ++++++++++++++++++++++++++++++++++++++++++++++
 arm/unittests.cfg |   6 ++
 2 files changed, 143 insertions(+)

diff --git a/arm/pmu.c b/arm/pmu.c
index fa77ab3..ada28a4 100644
--- a/arm/pmu.c
+++ b/arm/pmu.c
@@ -45,6 +45,11 @@ struct pmu {
 	uint32_t pmcr_ro;
 };
 
+struct pmu_stats {
+	unsigned long bitmap;
+	uint32_t interrupts[32];
+};
+
 static struct pmu pmu;
 
 #if defined(__arm__)
@@ -116,6 +121,7 @@ static void test_mem_access(void) {}
 static void test_chained_counters(void) {}
 static void test_chained_sw_incr(void) {}
 static void test_chain_promotion(void) {}
+static void test_overflow_interrupt(void) {}
 
 #elif defined(__aarch64__)
 #define ID_AA64DFR0_PERFMON_SHIFT 8
@@ -261,6 +267,44 @@ asm volatile(
 	: "x9", "x10", "cc");
 }
 
+static struct pmu_stats pmu_stats;
+
+static void irq_handler(struct pt_regs *regs)
+{
+	uint32_t irqstat, irqnr;
+
+	irqstat = gic_read_iar();
+	irqnr = gic_iar_irqnr(irqstat);
+	gic_write_eoir(irqstat);
+
+	if (irqnr == 23) {
+		unsigned long overflows = read_sysreg(pmovsclr_el0);
+		int i;
+
+		report_info("--> PMU overflow interrupt %d (counter bitmask 0x%lx)",
+			    irqnr, overflows);
+		for (i = 0; i < 32; i++) {
+			if (test_and_clear_bit(i, &overflows)) {
+				pmu_stats.interrupts[i]++;
+				pmu_stats.bitmap |= 1 << i;
+			}
+		}
+		write_sysreg(0xFFFFFFFF, pmovsclr_el0);
+	} else {
+		report_info("Unexpected interrupt: %d\n", irqnr);
+	}
+}
+
+static void pmu_reset_stats(void)
+{
+	int i;
+
+	for (i = 0; i < 32; i++)
+		pmu_stats.interrupts[i] = 0;
+
+	pmu_stats.bitmap = 0;
+}
+
 static void pmu_reset(void)
 {
 	/* reset all counters, counting disabled at PMCR level*/
@@ -271,6 +315,7 @@ static void pmu_reset(void)
 	write_sysreg(0xFFFFFFFF, pmovsclr_el0);
 	/* disable overflow interrupts on all counters */
 	write_sysreg(0xFFFFFFFF, pmintenclr_el1);
+	pmu_reset_stats();
 	isb();
 }
 
@@ -713,6 +758,95 @@ static void test_chain_promotion(void)
 			read_sysreg(pmovsclr_el0));
 }
 
+static bool expect_interrupts(uint32_t bitmap)
+{
+	int i;
+
+	if (pmu_stats.bitmap ^ bitmap)
+		return false;
+
+	for (i = 0; i < 32; i++) {
+		if (test_and_clear_bit(i, &pmu_stats.bitmap))
+			if (pmu_stats.interrupts[i] != 1)
+				return false;
+	}
+	return true;
+}
+
+static void test_overflow_interrupt(void)
+{
+	uint32_t events[] = { 0x13 /* MEM_ACCESS */, 0x00 /* SW_INCR */};
+	void *addr = malloc(PAGE_SIZE);
+	int i;
+
+	if (!satisfy_prerequisites(events, ARRAY_SIZE(events)))
+		return;
+
+	gic_enable_defaults();
+	install_irq_handler(EL1H_IRQ, irq_handler);
+	local_irq_enable();
+	gic_enable_irq(23);
+
+	pmu_reset();
+
+	write_regn(pmevtyper, 0, events[0] | PMEVTYPER_EXCLUDE_EL0);
+	write_regn(pmevtyper, 1, events[1] | PMEVTYPER_EXCLUDE_EL0);
+	write_sysreg_s(0x3, PMCNTENSET_EL0);
+	write_regn(pmevcntr, 0, 0xFFFFFFF0);
+	write_regn(pmevcntr, 1, 0xFFFFFFF0);
+	isb();
+
+	/* interrupts are disabled */
+
+	mem_access_loop(addr, 200, pmu.pmcr_ro | PMU_PMCR_E);
+	report(expect_interrupts(0), "no overflow interrupt received");
+
+	set_pmcr(pmu.pmcr_ro | PMU_PMCR_E);
+	for (i = 0; i < 100; i++)
+		write_sysreg(0x2, pmswinc_el0);
+
+	set_pmcr(pmu.pmcr_ro);
+	report(expect_interrupts(0), "no overflow interrupt received");
+
+	/* enable interrupts */
+
+	pmu_reset_stats();
+
+	write_regn(pmevcntr, 0, 0xFFFFFFF0);
+	write_regn(pmevcntr, 1, 0xFFFFFFF0);
+	write_sysreg(0xFFFFFFFF, pmintenset_el1);
+	isb();
+
+	mem_access_loop(addr, 200, pmu.pmcr_ro | PMU_PMCR_E);
+	for (i = 0; i < 100; i++)
+		write_sysreg(0x3, pmswinc_el0);
+
+	mem_access_loop(addr, 200, pmu.pmcr_ro);
+	report_info("overflow=0x%lx", read_sysreg(pmovsclr_el0));
+	report(expect_interrupts(0x3),
+		"overflow interrupts expected on #0 and #1");
+
+	/* promote to 64-b */
+
+	pmu_reset_stats();
+
+	events[1] = 0x1E /* CHAIN */;
+	write_regn(pmevtyper, 1, events[1] | PMEVTYPER_EXCLUDE_EL0);
+	write_regn(pmevcntr, 0, 0xFFFFFFF0);
+	isb();
+	mem_access_loop(addr, 200, pmu.pmcr_ro | PMU_PMCR_E);
+	report(expect_interrupts(0),
+		"no overflow interrupt expected on 32b boundary");
+
+	/* overflow on odd counter */
+	pmu_reset_stats();
+	write_regn(pmevcntr, 0, 0xFFFFFFF0);
+	write_regn(pmevcntr, 1, 0xFFFFFFFF);
+	isb();
+	mem_access_loop(addr, 400, pmu.pmcr_ro | PMU_PMCR_E);
+	report(expect_interrupts(0x2),
+		"expect overflow interrupt on odd counter");
+}
 #endif
 
 /*
@@ -921,6 +1055,9 @@ int main(int argc, char *argv[])
 	} else if (strcmp(argv[1], "chain-promotion") == 0) {
 		report_prefix_push(argv[1]);
 		test_chain_promotion();
+	} else if (strcmp(argv[1], "overflow-interrupt") == 0) {
+		report_prefix_push(argv[1]);
+		test_overflow_interrupt();
 	} else {
 		report_abort("Unknown sub-test '%s'", argv[1]);
 	}
diff --git a/arm/unittests.cfg b/arm/unittests.cfg
index eb6e87e..1d1bc27 100644
--- a/arm/unittests.cfg
+++ b/arm/unittests.cfg
@@ -108,6 +108,12 @@ groups = pmu
 arch = arm64
 extra_params = -append 'chain-promotion'
 
+[overflow-interrupt]
+file = pmu.flat
+groups = pmu
+arch = arm64
+extra_params = -append 'overflow-interrupt'
+
 # Test PMU support (TCG) with -icount IPC=1
 #[pmu-tcg-icount-1]
 #file = pmu.flat
-- 
2.20.1

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [kvm-unit-tests PATCH v2 9/9] arm: pmu: Test overflow interrupts

[Andrew Jones]

On Thu, Jan 30, 2020 at 12:25:10PM +0100, Eric Auger wrote:
> Test overflows for MEM_ACCESS and SW_INCR events. Also tests
> overflows with 64-bit events.
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> 
> ---
> 
> v1 -> v2:
> - inline setup_irq() code
> ---
>  arm/pmu.c         | 137 ++++++++++++++++++++++++++++++++++++++++++++++
>  arm/unittests.cfg |   6 ++
>  2 files changed, 143 insertions(+)
> 
> diff --git a/arm/pmu.c b/arm/pmu.c
> index fa77ab3..ada28a4 100644
> --- a/arm/pmu.c
> +++ b/arm/pmu.c
> @@ -45,6 +45,11 @@ struct pmu {
>  	uint32_t pmcr_ro;
>  };
>  
> +struct pmu_stats {
> +	unsigned long bitmap;
> +	uint32_t interrupts[32];
> +};
> +
>  static struct pmu pmu;
>  
>  #if defined(__arm__)
> @@ -116,6 +121,7 @@ static void test_mem_access(void) {}
>  static void test_chained_counters(void) {}
>  static void test_chained_sw_incr(void) {}
>  static void test_chain_promotion(void) {}
> +static void test_overflow_interrupt(void) {}
>  
>  #elif defined(__aarch64__)
>  #define ID_AA64DFR0_PERFMON_SHIFT 8
> @@ -261,6 +267,44 @@ asm volatile(
>  	: "x9", "x10", "cc");
>  }
>  
> +static struct pmu_stats pmu_stats;
> +
> +static void irq_handler(struct pt_regs *regs)
> +{
> +	uint32_t irqstat, irqnr;
> +
> +	irqstat = gic_read_iar();
> +	irqnr = gic_iar_irqnr(irqstat);
> +	gic_write_eoir(irqstat);

Should we clear the overflows before EOIRing? Otherwise I think it may be
possible for another interrupt to be delivered. See

https://patchwork.kernel.org/patch/11368853/

for a similar issue.

> +
> +	if (irqnr == 23) {

Why 23? And how about a define?

> +		unsigned long overflows = read_sysreg(pmovsclr_el0);
> +		int i;
> +
> +		report_info("--> PMU overflow interrupt %d (counter bitmask 0x%lx)",
> +			    irqnr, overflows);
> +		for (i = 0; i < 32; i++) {
> +			if (test_and_clear_bit(i, &overflows)) {
> +				pmu_stats.interrupts[i]++;
> +				pmu_stats.bitmap |= 1 << i;
> +			}
> +		}
> +		write_sysreg(0xFFFFFFFF, pmovsclr_el0);
> +	} else {
> +		report_info("Unexpected interrupt: %d\n", irqnr);

We should probably avoid calling any print functions from interrupt
handlers. I see the timer test irq handler has this too, though. Also
the pl031 test has a bunch of reporting in its irq handler. We do
better with the gic tests where we only write results to arrays and
then report later.

> +	}
> +}
> +
> +static void pmu_reset_stats(void)
> +{
> +	int i;
> +
> +	for (i = 0; i < 32; i++)
> +		pmu_stats.interrupts[i] = 0;
> +
> +	pmu_stats.bitmap = 0;
> +}
> +
>  static void pmu_reset(void)
>  {
>  	/* reset all counters, counting disabled at PMCR level*/
> @@ -271,6 +315,7 @@ static void pmu_reset(void)
>  	write_sysreg(0xFFFFFFFF, pmovsclr_el0);
>  	/* disable overflow interrupts on all counters */
>  	write_sysreg(0xFFFFFFFF, pmintenclr_el1);
> +	pmu_reset_stats();
>  	isb();
>  }
>  
> @@ -713,6 +758,95 @@ static void test_chain_promotion(void)
>  			read_sysreg(pmovsclr_el0));
>  }
>  
> +static bool expect_interrupts(uint32_t bitmap)
> +{
> +	int i;
> +
> +	if (pmu_stats.bitmap ^ bitmap)
> +		return false;
> +
> +	for (i = 0; i < 32; i++) {
> +		if (test_and_clear_bit(i, &pmu_stats.bitmap))
> +			if (pmu_stats.interrupts[i] != 1)
> +				return false;
> +	}
> +	return true;
> +}
> +
> +static void test_overflow_interrupt(void)
> +{
> +	uint32_t events[] = { 0x13 /* MEM_ACCESS */, 0x00 /* SW_INCR */};
> +	void *addr = malloc(PAGE_SIZE);
> +	int i;
> +
> +	if (!satisfy_prerequisites(events, ARRAY_SIZE(events)))
> +		return;
> +
> +	gic_enable_defaults();
> +	install_irq_handler(EL1H_IRQ, irq_handler);
> +	local_irq_enable();
> +	gic_enable_irq(23);
> +
> +	pmu_reset();
> +
> +	write_regn(pmevtyper, 0, events[0] | PMEVTYPER_EXCLUDE_EL0);
> +	write_regn(pmevtyper, 1, events[1] | PMEVTYPER_EXCLUDE_EL0);
> +	write_sysreg_s(0x3, PMCNTENSET_EL0);
> +	write_regn(pmevcntr, 0, 0xFFFFFFF0);
> +	write_regn(pmevcntr, 1, 0xFFFFFFF0);
> +	isb();
> +
> +	/* interrupts are disabled */
> +
> +	mem_access_loop(addr, 200, pmu.pmcr_ro | PMU_PMCR_E);
> +	report(expect_interrupts(0), "no overflow interrupt received");
> +
> +	set_pmcr(pmu.pmcr_ro | PMU_PMCR_E);
> +	for (i = 0; i < 100; i++)
> +		write_sysreg(0x2, pmswinc_el0);
> +
> +	set_pmcr(pmu.pmcr_ro);
> +	report(expect_interrupts(0), "no overflow interrupt received");
> +
> +	/* enable interrupts */
> +
> +	pmu_reset_stats();
> +
> +	write_regn(pmevcntr, 0, 0xFFFFFFF0);
> +	write_regn(pmevcntr, 1, 0xFFFFFFF0);
> +	write_sysreg(0xFFFFFFFF, pmintenset_el1);
> +	isb();
> +
> +	mem_access_loop(addr, 200, pmu.pmcr_ro | PMU_PMCR_E);
> +	for (i = 0; i < 100; i++)
> +		write_sysreg(0x3, pmswinc_el0);
> +
> +	mem_access_loop(addr, 200, pmu.pmcr_ro);
> +	report_info("overflow=0x%lx", read_sysreg(pmovsclr_el0));
> +	report(expect_interrupts(0x3),
> +		"overflow interrupts expected on #0 and #1");
> +
> +	/* promote to 64-b */
> +
> +	pmu_reset_stats();
> +
> +	events[1] = 0x1E /* CHAIN */;
> +	write_regn(pmevtyper, 1, events[1] | PMEVTYPER_EXCLUDE_EL0);
> +	write_regn(pmevcntr, 0, 0xFFFFFFF0);
> +	isb();
> +	mem_access_loop(addr, 200, pmu.pmcr_ro | PMU_PMCR_E);
> +	report(expect_interrupts(0),
> +		"no overflow interrupt expected on 32b boundary");
> +
> +	/* overflow on odd counter */
> +	pmu_reset_stats();
> +	write_regn(pmevcntr, 0, 0xFFFFFFF0);
> +	write_regn(pmevcntr, 1, 0xFFFFFFFF);
> +	isb();
> +	mem_access_loop(addr, 400, pmu.pmcr_ro | PMU_PMCR_E);
> +	report(expect_interrupts(0x2),
> +		"expect overflow interrupt on odd counter");
> +}
>  #endif
>  
>  /*
> @@ -921,6 +1055,9 @@ int main(int argc, char *argv[])
>  	} else if (strcmp(argv[1], "chain-promotion") == 0) {
>  		report_prefix_push(argv[1]);
>  		test_chain_promotion();
> +	} else if (strcmp(argv[1], "overflow-interrupt") == 0) {
> +		report_prefix_push(argv[1]);
> +		test_overflow_interrupt();
>  	} else {
>  		report_abort("Unknown sub-test '%s'", argv[1]);
>  	}
> diff --git a/arm/unittests.cfg b/arm/unittests.cfg
> index eb6e87e..1d1bc27 100644
> --- a/arm/unittests.cfg
> +++ b/arm/unittests.cfg
> @@ -108,6 +108,12 @@ groups = pmu
>  arch = arm64
>  extra_params = -append 'chain-promotion'
>  
> +[overflow-interrupt]

Need "pmu-" prefix on this name, like the others, otherwise its standalone
test won't have an appropriate name.

> +file = pmu.flat
> +groups = pmu
> +arch = arm64
> +extra_params = -append 'overflow-interrupt'
> +
>  # Test PMU support (TCG) with -icount IPC=1
>  #[pmu-tcg-icount-1]
>  #file = pmu.flat
> -- 
> 2.20.1
> 
> 

also same comments as previous patches

Thanks,
drew

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [kvm-unit-tests PATCH v2 0/9] KVM: arm64: PMUv3 Event Counter Tests

[Peter Maydell]

On Thu, 30 Jan 2020 at 11:25, Eric Auger <eric.auger@redhat.com> wrote:
>
> This series implements tests exercising the PMUv3 event counters.
> It tests both the 32-bit and 64-bit versions. Overflow interrupts
> also are checked. Those tests only are written for arm64.
>
> It allowed to reveal some issues related to SW_INCR implementation
> (esp. related to 64-bit implementation), some problems related to
> 32-bit <-> 64-bit transitions and consistency of enabled states
> of odd and event counters (See [1]).
>
> Overflow interrupt testing relies of one patch from Andre
> ("arm: gic: Provide per-IRQ helper functions") to enable the
> PPI 23, coming from "arm: gic: Test SPIs and interrupt groups"
> (https://patchwork.kernel.org/cover/11234975/). Drew kindly
> provided "arm64: Provide read/write_sysreg_s".
>
> All PMU tests can be launched with:
> ./run_tests.sh -g pmu
> Tests also can be launched individually. For example:
> ./arm-run arm/pmu.flat -append 'chained-sw-incr'
>
> With KVM:
> - chain-promotion and chained-sw-incr are known to be failing.
>   [1] proposed a fix.
> - On TX2, I have some random failures due to MEM_ACCESS event
>   measured with a great disparity. This is not observed on
>   other machines I have access to.
> With TCG:
> - all new tests are skipped

I'm having a go at using this patchset to test the support
I'm adding for TCG for the v8.1 and v8.4 PMU extensions...

Q1: how can I get run_tests.sh to pass extra arguments to
QEMU ? The PMU events check will fail unless QEMU gets
the '-icount 8' to enable cycle-counting, but although
the underlying ./arm/run lets you add arbitrary extra
arguments to QEMU, run_tests.sh doesn't seem to. Trying to
pass them in via "QEMU=/path/to/qemu -icount 8" doesn't
work either.

Q2: do you know why arm/pmu.c:check_pmcr() insists that
PMCR.IMP is non-zero? The comment says "simple sanity check",
but architecturally a zero IMP field is permitted (meaning
"go look at MIDR_EL1 instead"). This causes TCG to fail this
test on '-cpu max', because in that case we set PMCR.IMP
to the same thing as MIDR_EL1.Implementer which is 0
("software use", since QEMU is software...)

thanks
-- PMM
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [kvm-unit-tests PATCH v2 0/9] KVM: arm64: PMUv3 Event Counter Tests

[Andrew Jones]

On Tue, Feb 11, 2020 at 03:42:38PM +0000, Peter Maydell wrote:
> On Thu, 30 Jan 2020 at 11:25, Eric Auger <eric.auger@redhat.com> wrote:
> >
> > This series implements tests exercising the PMUv3 event counters.
> > It tests both the 32-bit and 64-bit versions. Overflow interrupts
> > also are checked. Those tests only are written for arm64.
> >
> > It allowed to reveal some issues related to SW_INCR implementation
> > (esp. related to 64-bit implementation), some problems related to
> > 32-bit <-> 64-bit transitions and consistency of enabled states
> > of odd and event counters (See [1]).
> >
> > Overflow interrupt testing relies of one patch from Andre
> > ("arm: gic: Provide per-IRQ helper functions") to enable the
> > PPI 23, coming from "arm: gic: Test SPIs and interrupt groups"
> > (https://patchwork.kernel.org/cover/11234975/). Drew kindly
> > provided "arm64: Provide read/write_sysreg_s".
> >
> > All PMU tests can be launched with:
> > ./run_tests.sh -g pmu
> > Tests also can be launched individually. For example:
> > ./arm-run arm/pmu.flat -append 'chained-sw-incr'
> >
> > With KVM:
> > - chain-promotion and chained-sw-incr are known to be failing.
> >   [1] proposed a fix.
> > - On TX2, I have some random failures due to MEM_ACCESS event
> >   measured with a great disparity. This is not observed on
> >   other machines I have access to.
> > With TCG:
> > - all new tests are skipped
> 
> I'm having a go at using this patchset to test the support
> I'm adding for TCG for the v8.1 and v8.4 PMU extensions...
> 
> Q1: how can I get run_tests.sh to pass extra arguments to
> QEMU ? The PMU events check will fail unless QEMU gets
> the '-icount 8' to enable cycle-counting, but although
> the underlying ./arm/run lets you add arbitrary extra
> arguments to QEMU, run_tests.sh doesn't seem to. Trying to
> pass them in via "QEMU=/path/to/qemu -icount 8" doesn't
> work either.

Alex Bennee once submit a patch[*] allowing that to work, but
it never got merged. I just rebased it and tried it, but it
doesn't work now. Too much has changed in the run scripts
since his posting. I can try to rework it though.

[*] https://github.com/rhdrjones/kvm-unit-tests/commit/9a8574bfd924f3e865611688e26bb12e53821747

> 
> Q2: do you know why arm/pmu.c:check_pmcr() insists that
> PMCR.IMP is non-zero? The comment says "simple sanity check",
> but architecturally a zero IMP field is permitted (meaning
> "go look at MIDR_EL1 instead"). This causes TCG to fail this
> test on '-cpu max', because in that case we set PMCR.IMP
> to the same thing as MIDR_EL1.Implementer which is 0
> ("software use", since QEMU is software...)

Probably just a misunderstanding on the part of the author (and
reviewers). Maybe Eric can fix that while preparing this series.

Thanks,
drew

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [kvm-unit-tests PATCH v2 0/9] KVM: arm64: PMUv3 Event Counter Tests

[Auger Eric]

On 2/11/20 5:07 PM, Andrew Jones wrote:
> On Tue, Feb 11, 2020 at 03:42:38PM +0000, Peter Maydell wrote:
>> On Thu, 30 Jan 2020 at 11:25, Eric Auger <eric.auger@redhat.com> wrote:
>>>
>>> This series implements tests exercising the PMUv3 event counters.
>>> It tests both the 32-bit and 64-bit versions. Overflow interrupts
>>> also are checked. Those tests only are written for arm64.
>>>
>>> It allowed to reveal some issues related to SW_INCR implementation
>>> (esp. related to 64-bit implementation), some problems related to
>>> 32-bit <-> 64-bit transitions and consistency of enabled states
>>> of odd and event counters (See [1]).
>>>
>>> Overflow interrupt testing relies of one patch from Andre
>>> ("arm: gic: Provide per-IRQ helper functions") to enable the
>>> PPI 23, coming from "arm: gic: Test SPIs and interrupt groups"
>>> (https://patchwork.kernel.org/cover/11234975/). Drew kindly
>>> provided "arm64: Provide read/write_sysreg_s".
>>>
>>> All PMU tests can be launched with:
>>> ./run_tests.sh -g pmu
>>> Tests also can be launched individually. For example:
>>> ./arm-run arm/pmu.flat -append 'chained-sw-incr'
>>>
>>> With KVM:
>>> - chain-promotion and chained-sw-incr are known to be failing.
>>>   [1] proposed a fix.
>>> - On TX2, I have some random failures due to MEM_ACCESS event
>>>   measured with a great disparity. This is not observed on
>>>   other machines I have access to.
>>> With TCG:
>>> - all new tests are skipped
>>
>> I'm having a go at using this patchset to test the support
>> I'm adding for TCG for the v8.1 and v8.4 PMU extensions...
>>
>> Q1: how can I get run_tests.sh to pass extra arguments to
>> QEMU ? The PMU events check will fail unless QEMU gets
>> the '-icount 8' to enable cycle-counting, but although
>> the underlying ./arm/run lets you add arbitrary extra
>> arguments to QEMU, run_tests.sh doesn't seem to. Trying to
>> pass them in via "QEMU=/path/to/qemu -icount 8" doesn't
>> work either.
int arm/unittests.cfg

there are tests related to TCG that are commented.

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

I wondered why we kept those and commented. Should we start with
separate tests for KVM and TCG?

> 
> Alex Bennee once submit a patch[*] allowing that to work, but
> it never got merged. I just rebased it and tried it, but it
> doesn't work now. Too much has changed in the run scripts
> since his posting. I can try to rework it though.
> 
> [*] https://github.com/rhdrjones/kvm-unit-tests/commit/9a8574bfd924f3e865611688e26bb12e53821747
> 
>>
>> Q2: do you know why arm/pmu.c:check_pmcr() insists that
>> PMCR.IMP is non-zero? The comment says "simple sanity check",
>> but architecturally a zero IMP field is permitted (meaning
>> "go look at MIDR_EL1 instead"). This causes TCG to fail this
>> test on '-cpu max', because in that case we set PMCR.IMP
>> to the same thing as MIDR_EL1.Implementer which is 0
>> ("software use", since QEMU is software...)
indeed
> 
> Probably just a misunderstanding on the part of the author (and
> reviewers). Maybe Eric can fix that while preparing this series.

Yes I can definitively fix that

Thanks

Eric
> 
> Thanks,
> drew
> 

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm