[kvm-unit-tests PATCH 0/3] arm: pmu: Add support for PMUv3p5

[kvm-unit-tests PATCH 0/3] arm: pmu: Add support for PMUv3p5

[Ricardo Koller]

The first commit fixes the tests when running on PMUv3p5. The issue is that
PMUv3p5 uses 64-bit counters irrespective of whether the PMU is configured
for overflowing at 32 or 64-bits. Tests are currently failing [0] on
PMUv3p5 because of this. They wrongly assume that values will be wrapped
around 32-bits, but they overflow into the other half of the 64-bit
counters.

The second and third commits add new tests for 64-bit overflows, a feature
added with PMUv3p5 (PMCR_EL0.LP == 1). This is done by running all
overflow-related tests in two modes: with 32-bit and 64-bit overflows.

This series was tested on PMUv3p5 and PMUv3p4 using the ARM Fast Model and
kvmtool.  All tests pass on both PMUv3p5 and PMUv3p4 when using Marc's
PMUv3p5 series [0], plus the suggestion made at [1]. Didn't test AArch32.

[0] https://lore.kernel.org/kvmarm/20221113163832.3154370-1-maz@kernel.org/
[1] https://lore.kernel.org/kvmarm/Y4jasyxvFRNvvmox@google.com/

Ricardo Koller (3):
  arm: pmu: Fix overflow checks for PMUv3p5 long counters
  arm: pmu: Prepare for testing 64-bit overflows
  arm: pmu: Add tests for 64-bit overflows

 arm/pmu.c | 217 +++++++++++++++++++++++++++++++++---------------------
 1 file changed, 133 insertions(+), 84 deletions(-)

-- 
2.39.0.rc0.267.gcb52ba06e7-goog

[kvm-unit-tests PATCH 1/3] arm: pmu: Fix overflow checks for PMUv3p5 long counters

[Ricardo Koller]

PMUv3p5 uses 64-bit counters irrespective of whether the PMU is configured
for overflowing at 32 or 64-bits. The consequence is that tests that check
the counter values after overflowing should not assume that values will be
wrapped around 32-bits: they overflow into the other half of the 64-bit
counters on PMUv3p5.

Fix tests by correctly checking overflowing-counters against the expected
64-bit value.

Signed-off-by: Ricardo Koller <ricarkol@google.com>
---
 arm/pmu.c | 29 ++++++++++++++++++-----------
 1 file changed, 18 insertions(+), 11 deletions(-)

diff --git a/arm/pmu.c b/arm/pmu.c
index cd47b14..eeac984 100644
--- a/arm/pmu.c
+++ b/arm/pmu.c
@@ -54,10 +54,10 @@
 #define EXT_COMMON_EVENTS_LOW	0x4000
 #define EXT_COMMON_EVENTS_HIGH	0x403F
 
-#define ALL_SET			0xFFFFFFFF
-#define ALL_CLEAR		0x0
-#define PRE_OVERFLOW		0xFFFFFFF0
-#define PRE_OVERFLOW2		0xFFFFFFDC
+#define ALL_SET			0x00000000FFFFFFFFULL
+#define ALL_CLEAR		0x0000000000000000ULL
+#define PRE_OVERFLOW		0x00000000FFFFFFF0ULL
+#define PRE_OVERFLOW2		0x00000000FFFFFFDCULL
 
 #define PMU_PPI			23
 
@@ -538,6 +538,7 @@ static void test_mem_access(void)
 static void test_sw_incr(void)
 {
 	uint32_t events[] = {SW_INCR, SW_INCR};
+	uint64_t cntr0;
 	int i;
 
 	if (!satisfy_prerequisites(events, ARRAY_SIZE(events)))
@@ -572,9 +573,9 @@ static void test_sw_incr(void)
 		write_sysreg(0x3, pmswinc_el0);
 
 	isb();
-	report(read_regn_el0(pmevcntr, 0)  == 84, "counter #1 after + 100 SW_INCR");
-	report(read_regn_el0(pmevcntr, 1)  == 100,
-		"counter #0 after + 100 SW_INCR");
+	cntr0 = (pmu.version < ID_DFR0_PMU_V3_8_5) ? 84 : PRE_OVERFLOW + 100;
+	report(read_regn_el0(pmevcntr, 0) == cntr0, "counter #0 after + 100 SW_INCR");
+	report(read_regn_el0(pmevcntr, 1) == 100, "counter #1 after + 100 SW_INCR");
 	report_info("counter values after 100 SW_INCR #0=%ld #1=%ld",
 		    read_regn_el0(pmevcntr, 0), read_regn_el0(pmevcntr, 1));
 	report(read_sysreg(pmovsclr_el0) == 0x1,
@@ -584,6 +585,7 @@ static void test_sw_incr(void)
 static void test_chained_counters(void)
 {
 	uint32_t events[] = {CPU_CYCLES, CHAIN};
+	uint64_t cntr1;
 
 	if (!satisfy_prerequisites(events, ARRAY_SIZE(events)))
 		return;
@@ -618,13 +620,16 @@ static void test_chained_counters(void)
 
 	precise_instrs_loop(22, pmu.pmcr_ro | PMU_PMCR_E);
 	report_info("overflow reg = 0x%lx", read_sysreg(pmovsclr_el0));
-	report(!read_regn_el0(pmevcntr, 1), "CHAIN counter #1 wrapped");
+	cntr1 = (pmu.version < ID_DFR0_PMU_V3_8_5) ? 0 : ALL_SET + 1;
+	report(read_regn_el0(pmevcntr, 1) == cntr1, "CHAIN counter #1 wrapped");
+
 	report(read_sysreg(pmovsclr_el0) == 0x3, "overflow on even and odd counters");
 }
 
 static void test_chained_sw_incr(void)
 {
 	uint32_t events[] = {SW_INCR, CHAIN};
+	uint64_t cntr0, cntr1;
 	int i;
 
 	if (!satisfy_prerequisites(events, ARRAY_SIZE(events)))
@@ -665,10 +670,12 @@ static void test_chained_sw_incr(void)
 		write_sysreg(0x1, pmswinc_el0);
 
 	isb();
+	cntr0 = (pmu.version < ID_DFR0_PMU_V3_8_5) ? 0 : ALL_SET + 1;
+	cntr1 = (pmu.version < ID_DFR0_PMU_V3_8_5) ? 84 : PRE_OVERFLOW + 100;
 	report((read_sysreg(pmovsclr_el0) == 0x3) &&
-		(read_regn_el0(pmevcntr, 1) == 0) &&
-		(read_regn_el0(pmevcntr, 0) == 84),
-		"expected overflows and values after 100 SW_INCR/CHAIN");
+	       (read_regn_el0(pmevcntr, 1) == cntr0) &&
+	       (read_regn_el0(pmevcntr, 0) == cntr1),
+	       "expected overflows and values after 100 SW_INCR/CHAIN");
 	report_info("overflow=0x%lx, #0=%ld #1=%ld", read_sysreg(pmovsclr_el0),
 		    read_regn_el0(pmevcntr, 0), read_regn_el0(pmevcntr, 1));
 }
-- 
2.39.0.rc0.267.gcb52ba06e7-goog

Re: [kvm-unit-tests PATCH 1/3] arm: pmu: Fix overflow checks for PMUv3p5 long counters

[Alexandru Elisei]

Hi,

On Fri, Dec 02, 2022 at 04:55:25AM +0000, Ricardo Koller wrote:
> PMUv3p5 uses 64-bit counters irrespective of whether the PMU is configured
> for overflowing at 32 or 64-bits. The consequence is that tests that check
> the counter values after overflowing should not assume that values will be
> wrapped around 32-bits: they overflow into the other half of the 64-bit
> counters on PMUv3p5.
> 
> Fix tests by correctly checking overflowing-counters against the expected
> 64-bit value.
> 
> Signed-off-by: Ricardo Koller <ricarkol@google.com>
> ---
>  arm/pmu.c | 29 ++++++++++++++++++-----------
>  1 file changed, 18 insertions(+), 11 deletions(-)
> 
> diff --git a/arm/pmu.c b/arm/pmu.c
> index cd47b14..eeac984 100644
> --- a/arm/pmu.c
> +++ b/arm/pmu.c
> @@ -54,10 +54,10 @@
>  #define EXT_COMMON_EVENTS_LOW	0x4000
>  #define EXT_COMMON_EVENTS_HIGH	0x403F
>  
> -#define ALL_SET			0xFFFFFFFF
> -#define ALL_CLEAR		0x0
> -#define PRE_OVERFLOW		0xFFFFFFF0
> -#define PRE_OVERFLOW2		0xFFFFFFDC
> +#define ALL_SET			0x00000000FFFFFFFFULL
> +#define ALL_CLEAR		0x0000000000000000ULL
> +#define PRE_OVERFLOW		0x00000000FFFFFFF0ULL
> +#define PRE_OVERFLOW2		0x00000000FFFFFFDCULL
>  
>  #define PMU_PPI			23
>  
> @@ -538,6 +538,7 @@ static void test_mem_access(void)
>  static void test_sw_incr(void)
>  {
>  	uint32_t events[] = {SW_INCR, SW_INCR};
> +	uint64_t cntr0;
>  	int i;
>  
>  	if (!satisfy_prerequisites(events, ARRAY_SIZE(events)))
> @@ -572,9 +573,9 @@ static void test_sw_incr(void)
>  		write_sysreg(0x3, pmswinc_el0);
>  
>  	isb();
> -	report(read_regn_el0(pmevcntr, 0)  == 84, "counter #1 after + 100 SW_INCR");
> -	report(read_regn_el0(pmevcntr, 1)  == 100,
> -		"counter #0 after + 100 SW_INCR");
> +	cntr0 = (pmu.version < ID_DFR0_PMU_V3_8_5) ? 84 : PRE_OVERFLOW + 100;

Hm... in the Arm ARM it says that counters are 64-bit if PMUv3p5 is
implemented.  But it doesn't say anywhere that versions newer than p5 are
required to implement PMUv3p5.

For example, for PMUv3p7, it says that the feature is mandatory in Arm8.7
implementations. My interpretation of that is that it is not forbidden for
an implementer to cherry-pick this version on older versions of the
architecture where PMUv3p5 is not implemented.

Maybe the check should be pmu.version == ID_DFR0_PMU_V3_8_5, to match the
counter definitions in the architecture?

Also, I found the meaning of those numbers to be quite cryptic. Perhaps
something like this would be more resilient to changes to the value of
PRE_OVERFLOW and easier to understand:

+       cntr0 = (pmu.version < ID_DFR0_PMU_V3_8_5) ?
+               (uint32_t)PRE_OVERFLOW + 100 :
+               (uint64_t)PRE_OVERFLOW + 100;

I haven't tested the code, would that work?

Thanks,
Alex

> +	report(read_regn_el0(pmevcntr, 0) == cntr0, "counter #0 after + 100 SW_INCR");
> +	report(read_regn_el0(pmevcntr, 1) == 100, "counter #1 after + 100 SW_INCR");
>  	report_info("counter values after 100 SW_INCR #0=%ld #1=%ld",
>  		    read_regn_el0(pmevcntr, 0), read_regn_el0(pmevcntr, 1));
>  	report(read_sysreg(pmovsclr_el0) == 0x1,
> @@ -584,6 +585,7 @@ static void test_sw_incr(void)
>  static void test_chained_counters(void)
>  {
>  	uint32_t events[] = {CPU_CYCLES, CHAIN};
> +	uint64_t cntr1;
>  
>  	if (!satisfy_prerequisites(events, ARRAY_SIZE(events)))
>  		return;
> @@ -618,13 +620,16 @@ static void test_chained_counters(void)
>  
>  	precise_instrs_loop(22, pmu.pmcr_ro | PMU_PMCR_E);
>  	report_info("overflow reg = 0x%lx", read_sysreg(pmovsclr_el0));
> -	report(!read_regn_el0(pmevcntr, 1), "CHAIN counter #1 wrapped");
> +	cntr1 = (pmu.version < ID_DFR0_PMU_V3_8_5) ? 0 : ALL_SET + 1;
> +	report(read_regn_el0(pmevcntr, 1) == cntr1, "CHAIN counter #1 wrapped");
> +
>  	report(read_sysreg(pmovsclr_el0) == 0x3, "overflow on even and odd counters");
>  }
>  
>  static void test_chained_sw_incr(void)
>  {
>  	uint32_t events[] = {SW_INCR, CHAIN};
> +	uint64_t cntr0, cntr1;
>  	int i;
>  
>  	if (!satisfy_prerequisites(events, ARRAY_SIZE(events)))
> @@ -665,10 +670,12 @@ static void test_chained_sw_incr(void)
>  		write_sysreg(0x1, pmswinc_el0);
>  
>  	isb();
> +	cntr0 = (pmu.version < ID_DFR0_PMU_V3_8_5) ? 0 : ALL_SET + 1;
> +	cntr1 = (pmu.version < ID_DFR0_PMU_V3_8_5) ? 84 : PRE_OVERFLOW + 100;
>  	report((read_sysreg(pmovsclr_el0) == 0x3) &&
> -		(read_regn_el0(pmevcntr, 1) == 0) &&
> -		(read_regn_el0(pmevcntr, 0) == 84),
> -		"expected overflows and values after 100 SW_INCR/CHAIN");
> +	       (read_regn_el0(pmevcntr, 1) == cntr0) &&
> +	       (read_regn_el0(pmevcntr, 0) == cntr1),
> +	       "expected overflows and values after 100 SW_INCR/CHAIN");
>  	report_info("overflow=0x%lx, #0=%ld #1=%ld", read_sysreg(pmovsclr_el0),
>  		    read_regn_el0(pmevcntr, 0), read_regn_el0(pmevcntr, 1));
>  }
> -- 
> 2.39.0.rc0.267.gcb52ba06e7-goog
> 

Re: [kvm-unit-tests PATCH 1/3] arm: pmu: Fix overflow checks for PMUv3p5 long counters

[Marc Zyngier]

On Fri, 09 Dec 2022 17:47:14 +0000,
Alexandru Elisei <alexandru.elisei@arm.com> wrote:
> 
> Hi,
> 
> On Fri, Dec 02, 2022 at 04:55:25AM +0000, Ricardo Koller wrote:
> > PMUv3p5 uses 64-bit counters irrespective of whether the PMU is configured
> > for overflowing at 32 or 64-bits. The consequence is that tests that check
> > the counter values after overflowing should not assume that values will be
> > wrapped around 32-bits: they overflow into the other half of the 64-bit
> > counters on PMUv3p5.
> > 
> > Fix tests by correctly checking overflowing-counters against the expected
> > 64-bit value.
> > 
> > Signed-off-by: Ricardo Koller <ricarkol@google.com>
> > ---
> >  arm/pmu.c | 29 ++++++++++++++++++-----------
> >  1 file changed, 18 insertions(+), 11 deletions(-)
> > 
> > diff --git a/arm/pmu.c b/arm/pmu.c
> > index cd47b14..eeac984 100644
> > --- a/arm/pmu.c
> > +++ b/arm/pmu.c
> > @@ -54,10 +54,10 @@
> >  #define EXT_COMMON_EVENTS_LOW	0x4000
> >  #define EXT_COMMON_EVENTS_HIGH	0x403F
> >  
> > -#define ALL_SET			0xFFFFFFFF
> > -#define ALL_CLEAR		0x0
> > -#define PRE_OVERFLOW		0xFFFFFFF0
> > -#define PRE_OVERFLOW2		0xFFFFFFDC
> > +#define ALL_SET			0x00000000FFFFFFFFULL
> > +#define ALL_CLEAR		0x0000000000000000ULL
> > +#define PRE_OVERFLOW		0x00000000FFFFFFF0ULL
> > +#define PRE_OVERFLOW2		0x00000000FFFFFFDCULL
> >  
> >  #define PMU_PPI			23
> >  
> > @@ -538,6 +538,7 @@ static void test_mem_access(void)
> >  static void test_sw_incr(void)
> >  {
> >  	uint32_t events[] = {SW_INCR, SW_INCR};
> > +	uint64_t cntr0;
> >  	int i;
> >  
> >  	if (!satisfy_prerequisites(events, ARRAY_SIZE(events)))
> > @@ -572,9 +573,9 @@ static void test_sw_incr(void)
> >  		write_sysreg(0x3, pmswinc_el0);
> >  
> >  	isb();
> > -	report(read_regn_el0(pmevcntr, 0)  == 84, "counter #1 after + 100 SW_INCR");
> > -	report(read_regn_el0(pmevcntr, 1)  == 100,
> > -		"counter #0 after + 100 SW_INCR");
> > +	cntr0 = (pmu.version < ID_DFR0_PMU_V3_8_5) ? 84 : PRE_OVERFLOW + 100;
> 
> Hm... in the Arm ARM it says that counters are 64-bit if PMUv3p5 is
> implemented.  But it doesn't say anywhere that versions newer than p5 are
> required to implement PMUv3p5.

And I don't think it needs to say it, because there is otherwise no
way for SW to discover whether 64bit counters are implemented or not.

> 
> For example, for PMUv3p7, it says that the feature is mandatory in Arm8.7
> implementations. My interpretation of that is that it is not forbidden for
> an implementer to cherry-pick this version on older versions of the
> architecture where PMUv3p5 is not implemented.

I'm sorry to have to say that, but I find your suggestion that PMUv3p7
could be implemented without supporting the full gamut of PMUv3p5
ludicrous.

Please look back at the ARM ARM, specially at the tiny section titled
"Alternative ID scheme used for the Performance Monitors Extension
version" (DDI0487I.a, D17.1.3, page 5553), and the snipped of C code
that performs exactly this check:

<quote>
  if (value != 0xF and value >= number) {
  	// do something that relies on version 'number' of the feature
  }
</quote>

Replace 'value' with 7 (PMUv3p7), 'number' with 6 (PMUv3p5), and you
get the exact property that you pretend doesn't exist, allowing you to
rely on PMUv3p5 to be implemented when the HW has PMUv3p7.

> Maybe the check should be pmu.version == ID_DFR0_PMU_V3_8_5, to match the
> counter definitions in the architecture?

No, that'd be totally wrong. You need to check your understanding of
how the ID registers work.

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [kvm-unit-tests PATCH 1/3] arm: pmu: Fix overflow checks for PMUv3p5 long counters

[Alexandru Elisei]

Hi Marc,

On Sat, Dec 10, 2022 at 11:01:53AM +0000, Marc Zyngier wrote:
> On Fri, 09 Dec 2022 17:47:14 +0000,
> Alexandru Elisei <alexandru.elisei@arm.com> wrote:
> > 
> > Hi,
> > 
> > On Fri, Dec 02, 2022 at 04:55:25AM +0000, Ricardo Koller wrote:
> > > PMUv3p5 uses 64-bit counters irrespective of whether the PMU is configured
> > > for overflowing at 32 or 64-bits. The consequence is that tests that check
> > > the counter values after overflowing should not assume that values will be
> > > wrapped around 32-bits: they overflow into the other half of the 64-bit
> > > counters on PMUv3p5.
> > > 
> > > Fix tests by correctly checking overflowing-counters against the expected
> > > 64-bit value.
> > > 
> > > Signed-off-by: Ricardo Koller <ricarkol@google.com>
> > > ---
> > >  arm/pmu.c | 29 ++++++++++++++++++-----------
> > >  1 file changed, 18 insertions(+), 11 deletions(-)
> > > 
> > > diff --git a/arm/pmu.c b/arm/pmu.c
> > > index cd47b14..eeac984 100644
> > > --- a/arm/pmu.c
> > > +++ b/arm/pmu.c
> > > @@ -54,10 +54,10 @@
> > >  #define EXT_COMMON_EVENTS_LOW	0x4000
> > >  #define EXT_COMMON_EVENTS_HIGH	0x403F
> > >  
> > > -#define ALL_SET			0xFFFFFFFF
> > > -#define ALL_CLEAR		0x0
> > > -#define PRE_OVERFLOW		0xFFFFFFF0
> > > -#define PRE_OVERFLOW2		0xFFFFFFDC
> > > +#define ALL_SET			0x00000000FFFFFFFFULL
> > > +#define ALL_CLEAR		0x0000000000000000ULL
> > > +#define PRE_OVERFLOW		0x00000000FFFFFFF0ULL
> > > +#define PRE_OVERFLOW2		0x00000000FFFFFFDCULL
> > >  
> > >  #define PMU_PPI			23
> > >  
> > > @@ -538,6 +538,7 @@ static void test_mem_access(void)
> > >  static void test_sw_incr(void)
> > >  {
> > >  	uint32_t events[] = {SW_INCR, SW_INCR};
> > > +	uint64_t cntr0;
> > >  	int i;
> > >  
> > >  	if (!satisfy_prerequisites(events, ARRAY_SIZE(events)))
> > > @@ -572,9 +573,9 @@ static void test_sw_incr(void)
> > >  		write_sysreg(0x3, pmswinc_el0);
> > >  
> > >  	isb();
> > > -	report(read_regn_el0(pmevcntr, 0)  == 84, "counter #1 after + 100 SW_INCR");
> > > -	report(read_regn_el0(pmevcntr, 1)  == 100,
> > > -		"counter #0 after + 100 SW_INCR");
> > > +	cntr0 = (pmu.version < ID_DFR0_PMU_V3_8_5) ? 84 : PRE_OVERFLOW + 100;
> > 
> > Hm... in the Arm ARM it says that counters are 64-bit if PMUv3p5 is
> > implemented.  But it doesn't say anywhere that versions newer than p5 are
> > required to implement PMUv3p5.
> 
> And I don't think it needs to say it, because there is otherwise no
> way for SW to discover whether 64bit counters are implemented or not.
> 
> > 
> > For example, for PMUv3p7, it says that the feature is mandatory in Arm8.7
> > implementations. My interpretation of that is that it is not forbidden for
> > an implementer to cherry-pick this version on older versions of the
> > architecture where PMUv3p5 is not implemented.
> 
> I'm sorry to have to say that, but I find your suggestion that PMUv3p7
> could be implemented without supporting the full gamut of PMUv3p5
> ludicrous.
> 
> Please look back at the ARM ARM, specially at the tiny section titled
> "Alternative ID scheme used for the Performance Monitors Extension
> version" (DDI0487I.a, D17.1.3, page 5553), and the snipped of C code
> that performs exactly this check:
> 
> <quote>
>   if (value != 0xF and value >= number) {
>   	// do something that relies on version 'number' of the feature
>   }
> </quote>
> 
> Replace 'value' with 7 (PMUv3p7), 'number' with 6 (PMUv3p5), and you
> get the exact property that you pretend doesn't exist, allowing you to
> rely on PMUv3p5 to be implemented when the HW has PMUv3p7.
> 
> > Maybe the check should be pmu.version == ID_DFR0_PMU_V3_8_5, to match the
> > counter definitions in the architecture?
> 
> No, that'd be totally wrong. You need to check your understanding of
> how the ID registers work.

A simple "hey, you're wrong here, the PMU extensions do not follow the
principles of the ID scheme for fields in ID registers" would have
sufficed. Guess you never made a silly mistake ever, right?

Otherwise, good job encouraging people to help review KVM/arm64 patches ;)

Thanks,
Alex

Re: [kvm-unit-tests PATCH 1/3] arm: pmu: Fix overflow checks for PMUv3p5 long counters

[Marc Zyngier]

Alex,

On Sun, 11 Dec 2022 11:40:39 +0000,
Alexandru Elisei <alexandru.elisei@arm.com> wrote:
> 
> A simple "hey, you're wrong here, the PMU extensions do not follow the
> principles of the ID scheme for fields in ID registers" would have
> sufficed.

This is what I did, and saved you the hassle of looking it up.

> Guess you never made a silly mistake ever, right?

It's not so much about making a silly mistake. I do that all the time.
But it is about the way you state these things, and the weight that
your reviews carry. You're a trusted reviewer, with a lot of
experience, and posting with an @arm.com address: what you say in a
public forum sticks. When you assert that the author is wrong, they
will take it at face value.

> Otherwise, good job encouraging people to help review KVM/arm64 patches ;)

What is the worse: no review? or a review that spreads confusion?
Think about it. I'm all for being nice, but I will call bullshit when
I see it asserted by people with a certain level of authority.

And I've long made up my mind about the state of the KVM/arm64 review
process -- reviews rarely come from people who have volunteered to do
so, but instead from those who have either a vested interest in it, or
an ulterior motive. Hey ho...

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [kvm-unit-tests PATCH 1/3] arm: pmu: Fix overflow checks for PMUv3p5 long counters

[Alexandru Elisei]

Hi,

On Mon, Dec 12, 2022 at 09:05:02AM +0000, Marc Zyngier wrote:
> Alex,
> 
> On Sun, 11 Dec 2022 11:40:39 +0000,
> Alexandru Elisei <alexandru.elisei@arm.com> wrote:
> > 
> > A simple "hey, you're wrong here, the PMU extensions do not follow the
> > principles of the ID scheme for fields in ID registers" would have
> > sufficed.
> 
> This is what I did, and saved you the hassle of looking it up.

The comment was about how you went about it, not about proving someone
wrong. As expressive as it might be, I don't think that calling someone's
suggestion "ludicrous" from the position of authority associated with being
a maintainer is constructive; and can also be interpreted as a personal
attack (you used **your** suggestion, not **this** suggestion). I didn't
interpret it that way, just saying that it can be.

> 
> > Guess you never made a silly mistake ever, right?
> 
> It's not so much about making a silly mistake. I do that all the time.
> But it is about the way you state these things, and the weight that
> your reviews carry. You're a trusted reviewer, with a lot of
> experience, and posting with an @arm.com address: what you say in a
> public forum sticks. When you assert that the author is wrong, they
> will take it at face value.

This is how I stated things:

"Hm... in the Arm ARM it says that counters are 64-bit if PMUv3p5 is
implemented.  But it doesn't say anywhere that versions newer than p5 are
required to implement PMUv3p5." -> patently false, easily provable with the
Arm ARM and by logic (as you did). My entire argument was based on this, so
once this has been proven false, I would say that the rest of my argument
falls apart.

"For example, for PMUv3p7, it says that the feature is mandatory in Arm8.7
implementations. **My interpretation** of that is that it is not forbidden
for an implementer to cherry-pick this version on older versions of the
architecture where PMUv3p5 is not implemented." -> emphasis on the "my
interpretation"; also easy to prove false because PMUv3p5+ is required to
implement PMUv3p5, as per the architecture.

"**Maybe** the check should be pmu.version == ID_DFR0_PMU_V3_8_5, to match
the counter definitions in the architecture?" -> emphasis on the "maybe",
and the question mark at the end.

My intention wasn't to dictate something, my intention was to have a
conversation about the patch, with the mindset that I might be wrong. What
made you get the idea that I was asserting that the author is wrong? Where
by "asserting the author is wrong" I understand framing my comment in such
a way as to leave no room for further discussions. Or did you mean
something else by that?

Or, to put it another way, what about the way I stated things could have
been done better (other than not being wrong, obviously)?

> 
> > Otherwise, good job encouraging people to help review KVM/arm64 patches ;)
> 
> What is the worse: no review? or a review that spreads confusion?
> Think about it. I'm all for being nice, but I will call bullshit when

That wasn't about calling people out on their mistakes. I was saying that
the way you "call bullshit", as you put it, might be a put off for some
people. Call me naive, but I like to think that not everyone that comments
on a patch does it because they have to.

> I see it asserted by people with a certain level of authority.
> 
> And I've long made up my mind about the state of the KVM/arm64 review
> process -- reviews rarely come from people who have volunteered to do
> so, but instead from those who have either a vested interest in it, or
> an ulterior motive. Hey ho...

I genuinely don't know what to make of this. I can't even tell if it's
directed at me or not.

Thanks,
Alex

Re: [kvm-unit-tests PATCH 1/3] arm: pmu: Fix overflow checks for PMUv3p5 long counters

[Ricardo Koller]

On Fri, Dec 09, 2022 at 05:47:14PM +0000, Alexandru Elisei wrote:
> Hi,
> 
> On Fri, Dec 02, 2022 at 04:55:25AM +0000, Ricardo Koller wrote:
> > PMUv3p5 uses 64-bit counters irrespective of whether the PMU is configured
> > for overflowing at 32 or 64-bits. The consequence is that tests that check
> > the counter values after overflowing should not assume that values will be
> > wrapped around 32-bits: they overflow into the other half of the 64-bit
> > counters on PMUv3p5.
> > 
> > Fix tests by correctly checking overflowing-counters against the expected
> > 64-bit value.
> > 
> > Signed-off-by: Ricardo Koller <ricarkol@google.com>
> > ---
> >  arm/pmu.c | 29 ++++++++++++++++++-----------
> >  1 file changed, 18 insertions(+), 11 deletions(-)
> > 
> > diff --git a/arm/pmu.c b/arm/pmu.c
> > index cd47b14..eeac984 100644
> > --- a/arm/pmu.c
> > +++ b/arm/pmu.c
> > @@ -54,10 +54,10 @@
> >  #define EXT_COMMON_EVENTS_LOW	0x4000
> >  #define EXT_COMMON_EVENTS_HIGH	0x403F
> >  
> > -#define ALL_SET			0xFFFFFFFF
> > -#define ALL_CLEAR		0x0
> > -#define PRE_OVERFLOW		0xFFFFFFF0
> > -#define PRE_OVERFLOW2		0xFFFFFFDC
> > +#define ALL_SET			0x00000000FFFFFFFFULL
> > +#define ALL_CLEAR		0x0000000000000000ULL
> > +#define PRE_OVERFLOW		0x00000000FFFFFFF0ULL
> > +#define PRE_OVERFLOW2		0x00000000FFFFFFDCULL
> >  
> >  #define PMU_PPI			23
> >  
> > @@ -538,6 +538,7 @@ static void test_mem_access(void)
> >  static void test_sw_incr(void)
> >  {
> >  	uint32_t events[] = {SW_INCR, SW_INCR};
> > +	uint64_t cntr0;
> >  	int i;
> >  
> >  	if (!satisfy_prerequisites(events, ARRAY_SIZE(events)))
> > @@ -572,9 +573,9 @@ static void test_sw_incr(void)
> >  		write_sysreg(0x3, pmswinc_el0);
> >  
> >  	isb();
> > -	report(read_regn_el0(pmevcntr, 0)  == 84, "counter #1 after + 100 SW_INCR");
> > -	report(read_regn_el0(pmevcntr, 1)  == 100,
> > -		"counter #0 after + 100 SW_INCR");
> > +	cntr0 = (pmu.version < ID_DFR0_PMU_V3_8_5) ? 84 : PRE_OVERFLOW + 100;
> 
> Hm... in the Arm ARM it says that counters are 64-bit if PMUv3p5 is
> implemented.  But it doesn't say anywhere that versions newer than p5 are
> required to implement PMUv3p5.
> 
> For example, for PMUv3p7, it says that the feature is mandatory in Arm8.7
> implementations. My interpretation of that is that it is not forbidden for
> an implementer to cherry-pick this version on older versions of the
> architecture where PMUv3p5 is not implemented.
> 
> Maybe the check should be pmu.version == ID_DFR0_PMU_V3_8_5, to match the
> counter definitions in the architecture?
> 
> Also, I found the meaning of those numbers to be quite cryptic. Perhaps
> something like this would be more resilient to changes to the value of
> PRE_OVERFLOW and easier to understand:
> 
> +       cntr0 = (pmu.version < ID_DFR0_PMU_V3_8_5) ?
> +               (uint32_t)PRE_OVERFLOW + 100 :
> +               (uint64_t)PRE_OVERFLOW + 100;
> 
> I haven't tested the code, would that work?

Just checked. That works. It's much cleaner, thank you very much.

Will send a v2 a the end of the week, maybe after some more reviews.

> 
> Thanks,
> Alex
> 
> > +	report(read_regn_el0(pmevcntr, 0) == cntr0, "counter #0 after + 100 SW_INCR");
> > +	report(read_regn_el0(pmevcntr, 1) == 100, "counter #1 after + 100 SW_INCR");
> >  	report_info("counter values after 100 SW_INCR #0=%ld #1=%ld",
> >  		    read_regn_el0(pmevcntr, 0), read_regn_el0(pmevcntr, 1));
> >  	report(read_sysreg(pmovsclr_el0) == 0x1,
> > @@ -584,6 +585,7 @@ static void test_sw_incr(void)
> >  static void test_chained_counters(void)
> >  {
> >  	uint32_t events[] = {CPU_CYCLES, CHAIN};
> > +	uint64_t cntr1;
> >  
> >  	if (!satisfy_prerequisites(events, ARRAY_SIZE(events)))
> >  		return;
> > @@ -618,13 +620,16 @@ static void test_chained_counters(void)
> >  
> >  	precise_instrs_loop(22, pmu.pmcr_ro | PMU_PMCR_E);
> >  	report_info("overflow reg = 0x%lx", read_sysreg(pmovsclr_el0));
> > -	report(!read_regn_el0(pmevcntr, 1), "CHAIN counter #1 wrapped");
> > +	cntr1 = (pmu.version < ID_DFR0_PMU_V3_8_5) ? 0 : ALL_SET + 1;
> > +	report(read_regn_el0(pmevcntr, 1) == cntr1, "CHAIN counter #1 wrapped");
> > +
> >  	report(read_sysreg(pmovsclr_el0) == 0x3, "overflow on even and odd counters");
> >  }
> >  
> >  static void test_chained_sw_incr(void)
> >  {
> >  	uint32_t events[] = {SW_INCR, CHAIN};
> > +	uint64_t cntr0, cntr1;
> >  	int i;
> >  
> >  	if (!satisfy_prerequisites(events, ARRAY_SIZE(events)))
> > @@ -665,10 +670,12 @@ static void test_chained_sw_incr(void)
> >  		write_sysreg(0x1, pmswinc_el0);
> >  
> >  	isb();
> > +	cntr0 = (pmu.version < ID_DFR0_PMU_V3_8_5) ? 0 : ALL_SET + 1;
> > +	cntr1 = (pmu.version < ID_DFR0_PMU_V3_8_5) ? 84 : PRE_OVERFLOW + 100;
> >  	report((read_sysreg(pmovsclr_el0) == 0x3) &&
> > -		(read_regn_el0(pmevcntr, 1) == 0) &&
> > -		(read_regn_el0(pmevcntr, 0) == 84),
> > -		"expected overflows and values after 100 SW_INCR/CHAIN");
> > +	       (read_regn_el0(pmevcntr, 1) == cntr0) &&
> > +	       (read_regn_el0(pmevcntr, 0) == cntr1),
> > +	       "expected overflows and values after 100 SW_INCR/CHAIN");
> >  	report_info("overflow=0x%lx, #0=%ld #1=%ld", read_sysreg(pmovsclr_el0),
> >  		    read_regn_el0(pmevcntr, 0), read_regn_el0(pmevcntr, 1));
> >  }
> > -- 
> > 2.39.0.rc0.267.gcb52ba06e7-goog
> > 

Re: [kvm-unit-tests PATCH 1/3] arm: pmu: Fix overflow checks for PMUv3p5 long counters

[Alexandru Elisei]

Hi,

Some more comments below.

On Fri, Dec 02, 2022 at 04:55:25AM +0000, Ricardo Koller wrote:
> PMUv3p5 uses 64-bit counters irrespective of whether the PMU is configured
> for overflowing at 32 or 64-bits. The consequence is that tests that check
> the counter values after overflowing should not assume that values will be
> wrapped around 32-bits: they overflow into the other half of the 64-bit
> counters on PMUv3p5.
> 
> Fix tests by correctly checking overflowing-counters against the expected
> 64-bit value.
> 
> Signed-off-by: Ricardo Koller <ricarkol@google.com>
> ---
>  arm/pmu.c | 29 ++++++++++++++++++-----------
>  1 file changed, 18 insertions(+), 11 deletions(-)
> 
> diff --git a/arm/pmu.c b/arm/pmu.c
> index cd47b14..eeac984 100644
> --- a/arm/pmu.c
> +++ b/arm/pmu.c
> @@ -54,10 +54,10 @@
>  #define EXT_COMMON_EVENTS_LOW	0x4000
>  #define EXT_COMMON_EVENTS_HIGH	0x403F
>  
> -#define ALL_SET			0xFFFFFFFF
> -#define ALL_CLEAR		0x0
> -#define PRE_OVERFLOW		0xFFFFFFF0
> -#define PRE_OVERFLOW2		0xFFFFFFDC
> +#define ALL_SET			0x00000000FFFFFFFFULL
> +#define ALL_CLEAR		0x0000000000000000ULL
> +#define PRE_OVERFLOW		0x00000000FFFFFFF0ULL
> +#define PRE_OVERFLOW2		0x00000000FFFFFFDCULL
>  
>  #define PMU_PPI			23
>  
> @@ -538,6 +538,7 @@ static void test_mem_access(void)
>  static void test_sw_incr(void)
>  {
>  	uint32_t events[] = {SW_INCR, SW_INCR};
> +	uint64_t cntr0;
>  	int i;
>  
>  	if (!satisfy_prerequisites(events, ARRAY_SIZE(events)))
> @@ -572,9 +573,9 @@ static void test_sw_incr(void)
>  		write_sysreg(0x3, pmswinc_el0);
>  
>  	isb();
> -	report(read_regn_el0(pmevcntr, 0)  == 84, "counter #1 after + 100 SW_INCR");
> -	report(read_regn_el0(pmevcntr, 1)  == 100,
> -		"counter #0 after + 100 SW_INCR");
> +	cntr0 = (pmu.version < ID_DFR0_PMU_V3_8_5) ? 84 : PRE_OVERFLOW + 100;
> +	report(read_regn_el0(pmevcntr, 0) == cntr0, "counter #0 after + 100 SW_INCR");
> +	report(read_regn_el0(pmevcntr, 1) == 100, "counter #1 after + 100 SW_INCR");
>  	report_info("counter values after 100 SW_INCR #0=%ld #1=%ld",
>  		    read_regn_el0(pmevcntr, 0), read_regn_el0(pmevcntr, 1));
>  	report(read_sysreg(pmovsclr_el0) == 0x1,
> @@ -584,6 +585,7 @@ static void test_sw_incr(void)
>  static void test_chained_counters(void)
>  {
>  	uint32_t events[] = {CPU_CYCLES, CHAIN};
> +	uint64_t cntr1;
>  
>  	if (!satisfy_prerequisites(events, ARRAY_SIZE(events)))
>  		return;
> @@ -618,13 +620,16 @@ static void test_chained_counters(void)
>  
>  	precise_instrs_loop(22, pmu.pmcr_ro | PMU_PMCR_E);
>  	report_info("overflow reg = 0x%lx", read_sysreg(pmovsclr_el0));
> -	report(!read_regn_el0(pmevcntr, 1), "CHAIN counter #1 wrapped");
> +	cntr1 = (pmu.version < ID_DFR0_PMU_V3_8_5) ? 0 : ALL_SET + 1;
> +	report(read_regn_el0(pmevcntr, 1) == cntr1, "CHAIN counter #1 wrapped");

It looks to me like the intention of the test was to check that the counter
programmed with the CHAIN event wraps, judging from the report message.

I think it would be interesting to keep that by programming counter #1 with
~0ULL when PMUv3p5 (maybe call it ALL_SET64?) and test the counter value
against 0. Alternatively, the report message can be modified, and "wrapped"
replaced with "incremented" (or something like that), to avoid confusion.

> +
>  	report(read_sysreg(pmovsclr_el0) == 0x3, "overflow on even and odd counters");
>  }
>  
>  static void test_chained_sw_incr(void)
>  {
>  	uint32_t events[] = {SW_INCR, CHAIN};
> +	uint64_t cntr0, cntr1;
>  	int i;
>  
>  	if (!satisfy_prerequisites(events, ARRAY_SIZE(events)))
> @@ -665,10 +670,12 @@ static void test_chained_sw_incr(void)
>  		write_sysreg(0x1, pmswinc_el0);
>  
>  	isb();
> +	cntr0 = (pmu.version < ID_DFR0_PMU_V3_8_5) ? 0 : ALL_SET + 1;
> +	cntr1 = (pmu.version < ID_DFR0_PMU_V3_8_5) ? 84 : PRE_OVERFLOW + 100;
>  	report((read_sysreg(pmovsclr_el0) == 0x3) &&
> -		(read_regn_el0(pmevcntr, 1) == 0) &&
> -		(read_regn_el0(pmevcntr, 0) == 84),
> -		"expected overflows and values after 100 SW_INCR/CHAIN");
> +	       (read_regn_el0(pmevcntr, 1) == cntr0) &&
> +	       (read_regn_el0(pmevcntr, 0) == cntr1),

This is hard to parse, it would be better if counter 0 is compared against
cntr0 and counter 1 against cntr1.

Also, same suggestion here, looks like the test wants to check that the
odd-numbered counter wraps around when counting the CHAIN event.

Thanks,
Alex

> +	       "expected overflows and values after 100 SW_INCR/CHAIN");
>  	report_info("overflow=0x%lx, #0=%ld #1=%ld", read_sysreg(pmovsclr_el0),
>  		    read_regn_el0(pmevcntr, 0), read_regn_el0(pmevcntr, 1));
>  }
> -- 
> 2.39.0.rc0.267.gcb52ba06e7-goog
> 

Re: [kvm-unit-tests PATCH 1/3] arm: pmu: Fix overflow checks for PMUv3p5 long counters

[Ricardo Koller]

On Tue, Dec 13, 2022 at 12:36:14PM +0000, Alexandru Elisei wrote:
> Hi,
> 
> Some more comments below.
> 
> On Fri, Dec 02, 2022 at 04:55:25AM +0000, Ricardo Koller wrote:
> > PMUv3p5 uses 64-bit counters irrespective of whether the PMU is configured
> > for overflowing at 32 or 64-bits. The consequence is that tests that check
> > the counter values after overflowing should not assume that values will be
> > wrapped around 32-bits: they overflow into the other half of the 64-bit
> > counters on PMUv3p5.
> > 
> > Fix tests by correctly checking overflowing-counters against the expected
> > 64-bit value.
> > 
> > Signed-off-by: Ricardo Koller <ricarkol@google.com>
> > ---
> >  arm/pmu.c | 29 ++++++++++++++++++-----------
> >  1 file changed, 18 insertions(+), 11 deletions(-)
> > 
> > diff --git a/arm/pmu.c b/arm/pmu.c
> > index cd47b14..eeac984 100644
> > --- a/arm/pmu.c
> > +++ b/arm/pmu.c
> > @@ -54,10 +54,10 @@
> >  #define EXT_COMMON_EVENTS_LOW	0x4000
> >  #define EXT_COMMON_EVENTS_HIGH	0x403F
> >  
> > -#define ALL_SET			0xFFFFFFFF
> > -#define ALL_CLEAR		0x0
> > -#define PRE_OVERFLOW		0xFFFFFFF0
> > -#define PRE_OVERFLOW2		0xFFFFFFDC
> > +#define ALL_SET			0x00000000FFFFFFFFULL
> > +#define ALL_CLEAR		0x0000000000000000ULL
> > +#define PRE_OVERFLOW		0x00000000FFFFFFF0ULL
> > +#define PRE_OVERFLOW2		0x00000000FFFFFFDCULL
> >  
> >  #define PMU_PPI			23
> >  
> > @@ -538,6 +538,7 @@ static void test_mem_access(void)
> >  static void test_sw_incr(void)
> >  {
> >  	uint32_t events[] = {SW_INCR, SW_INCR};
> > +	uint64_t cntr0;
> >  	int i;
> >  
> >  	if (!satisfy_prerequisites(events, ARRAY_SIZE(events)))
> > @@ -572,9 +573,9 @@ static void test_sw_incr(void)
> >  		write_sysreg(0x3, pmswinc_el0);
> >  
> >  	isb();
> > -	report(read_regn_el0(pmevcntr, 0)  == 84, "counter #1 after + 100 SW_INCR");
> > -	report(read_regn_el0(pmevcntr, 1)  == 100,
> > -		"counter #0 after + 100 SW_INCR");
> > +	cntr0 = (pmu.version < ID_DFR0_PMU_V3_8_5) ? 84 : PRE_OVERFLOW + 100;
> > +	report(read_regn_el0(pmevcntr, 0) == cntr0, "counter #0 after + 100 SW_INCR");
> > +	report(read_regn_el0(pmevcntr, 1) == 100, "counter #1 after + 100 SW_INCR");
> >  	report_info("counter values after 100 SW_INCR #0=%ld #1=%ld",
> >  		    read_regn_el0(pmevcntr, 0), read_regn_el0(pmevcntr, 1));
> >  	report(read_sysreg(pmovsclr_el0) == 0x1,
> > @@ -584,6 +585,7 @@ static void test_sw_incr(void)
> >  static void test_chained_counters(void)
> >  {
> >  	uint32_t events[] = {CPU_CYCLES, CHAIN};
> > +	uint64_t cntr1;
> >  
> >  	if (!satisfy_prerequisites(events, ARRAY_SIZE(events)))
> >  		return;
> > @@ -618,13 +620,16 @@ static void test_chained_counters(void)
> >  
> >  	precise_instrs_loop(22, pmu.pmcr_ro | PMU_PMCR_E);
> >  	report_info("overflow reg = 0x%lx", read_sysreg(pmovsclr_el0));
> > -	report(!read_regn_el0(pmevcntr, 1), "CHAIN counter #1 wrapped");
> > +	cntr1 = (pmu.version < ID_DFR0_PMU_V3_8_5) ? 0 : ALL_SET + 1;
> > +	report(read_regn_el0(pmevcntr, 1) == cntr1, "CHAIN counter #1 wrapped");
> 
> It looks to me like the intention of the test was to check that the counter
> programmed with the CHAIN event wraps, judging from the report message.
> 

Ah, right. Yeah, that message is confusing. It should be the short
version of "Inrementing at 32-bits resulted in the right value".

> I think it would be interesting to keep that by programming counter #1 with
> ~0ULL when PMUv3p5 (maybe call it ALL_SET64?) and test the counter value
> against 0.

The last commit adds tests using ALL_SET64.  Tests can be run in two
modes: overflow_at_64bits and not.  However, this test,
test_chained_counters(), and all other chained tests only use the
!overflow_at_64bits mode (even after the last commit). The reason is
that there are no CHAIN events when overflowing at 64-bits (more details
in the commit message). But, don't worry, there are lots of tests that
check wrapping at 64-bits (overflow_at_64bits=true).

> Alternatively, the report message can be modified, and "wrapped"
> replaced with "incremented" (or something like that), to avoid confusion.
> 
> > +
> >  	report(read_sysreg(pmovsclr_el0) == 0x3, "overflow on even and odd counters");
> >  }
> >  
> >  static void test_chained_sw_incr(void)
> >  {
> >  	uint32_t events[] = {SW_INCR, CHAIN};
> > +	uint64_t cntr0, cntr1;
> >  	int i;
> >  
> >  	if (!satisfy_prerequisites(events, ARRAY_SIZE(events)))
> > @@ -665,10 +670,12 @@ static void test_chained_sw_incr(void)
> >  		write_sysreg(0x1, pmswinc_el0);
> >  
> >  	isb();
> > +	cntr0 = (pmu.version < ID_DFR0_PMU_V3_8_5) ? 0 : ALL_SET + 1;
> > +	cntr1 = (pmu.version < ID_DFR0_PMU_V3_8_5) ? 84 : PRE_OVERFLOW + 100;
> >  	report((read_sysreg(pmovsclr_el0) == 0x3) &&
> > -		(read_regn_el0(pmevcntr, 1) == 0) &&
> > -		(read_regn_el0(pmevcntr, 0) == 84),
> > -		"expected overflows and values after 100 SW_INCR/CHAIN");
> > +	       (read_regn_el0(pmevcntr, 1) == cntr0) &&
> > +	       (read_regn_el0(pmevcntr, 0) == cntr1),
> 
> This is hard to parse, it would be better if counter 0 is compared against
> cntr0 and counter 1 against cntr1.

Ah, yes, code is correct but that's indeed confusing.

> 
> Also, same suggestion here, looks like the test wants to check that the
> odd-numbered counter wraps around when counting the CHAIN event.

Ack. Will update for v2.

Thanks!
Ricardo

> 
> Thanks,
> Alex
> 
> > +	       "expected overflows and values after 100 SW_INCR/CHAIN");
> >  	report_info("overflow=0x%lx, #0=%ld #1=%ld", read_sysreg(pmovsclr_el0),
> >  		    read_regn_el0(pmevcntr, 0), read_regn_el0(pmevcntr, 1));
> >  }
> > -- 
> > 2.39.0.rc0.267.gcb52ba06e7-goog
> > 

Re: [kvm-unit-tests PATCH 1/3] arm: pmu: Fix overflow checks for PMUv3p5 long counters

[Alexandru Elisei]

Hi,

On Tue, Dec 13, 2022 at 08:21:24AM -0800, Ricardo Koller wrote:
> On Tue, Dec 13, 2022 at 12:36:14PM +0000, Alexandru Elisei wrote:
> > Hi,
> > 
> > Some more comments below.
> > 
> > On Fri, Dec 02, 2022 at 04:55:25AM +0000, Ricardo Koller wrote:
> > > PMUv3p5 uses 64-bit counters irrespective of whether the PMU is configured
> > > for overflowing at 32 or 64-bits. The consequence is that tests that check
> > > the counter values after overflowing should not assume that values will be
> > > wrapped around 32-bits: they overflow into the other half of the 64-bit
> > > counters on PMUv3p5.
> > > 
> > > Fix tests by correctly checking overflowing-counters against the expected
> > > 64-bit value.
> > > 
> > > Signed-off-by: Ricardo Koller <ricarkol@google.com>
> > > ---
> > >  arm/pmu.c | 29 ++++++++++++++++++-----------
> > >  1 file changed, 18 insertions(+), 11 deletions(-)
> > > 
> > > diff --git a/arm/pmu.c b/arm/pmu.c
> > > index cd47b14..eeac984 100644
> > > --- a/arm/pmu.c
> > > +++ b/arm/pmu.c
> > > @@ -54,10 +54,10 @@
> > >  #define EXT_COMMON_EVENTS_LOW	0x4000
> > >  #define EXT_COMMON_EVENTS_HIGH	0x403F
> > >  
> > > -#define ALL_SET			0xFFFFFFFF
> > > -#define ALL_CLEAR		0x0
> > > -#define PRE_OVERFLOW		0xFFFFFFF0
> > > -#define PRE_OVERFLOW2		0xFFFFFFDC
> > > +#define ALL_SET			0x00000000FFFFFFFFULL
> > > +#define ALL_CLEAR		0x0000000000000000ULL
> > > +#define PRE_OVERFLOW		0x00000000FFFFFFF0ULL
> > > +#define PRE_OVERFLOW2		0x00000000FFFFFFDCULL
> > >  
> > >  #define PMU_PPI			23
> > >  
> > > @@ -538,6 +538,7 @@ static void test_mem_access(void)
> > >  static void test_sw_incr(void)
> > >  {
> > >  	uint32_t events[] = {SW_INCR, SW_INCR};
> > > +	uint64_t cntr0;
> > >  	int i;
> > >  
> > >  	if (!satisfy_prerequisites(events, ARRAY_SIZE(events)))
> > > @@ -572,9 +573,9 @@ static void test_sw_incr(void)
> > >  		write_sysreg(0x3, pmswinc_el0);
> > >  
> > >  	isb();
> > > -	report(read_regn_el0(pmevcntr, 0)  == 84, "counter #1 after + 100 SW_INCR");
> > > -	report(read_regn_el0(pmevcntr, 1)  == 100,
> > > -		"counter #0 after + 100 SW_INCR");
> > > +	cntr0 = (pmu.version < ID_DFR0_PMU_V3_8_5) ? 84 : PRE_OVERFLOW + 100;
> > > +	report(read_regn_el0(pmevcntr, 0) == cntr0, "counter #0 after + 100 SW_INCR");
> > > +	report(read_regn_el0(pmevcntr, 1) == 100, "counter #1 after + 100 SW_INCR");
> > >  	report_info("counter values after 100 SW_INCR #0=%ld #1=%ld",
> > >  		    read_regn_el0(pmevcntr, 0), read_regn_el0(pmevcntr, 1));
> > >  	report(read_sysreg(pmovsclr_el0) == 0x1,
> > > @@ -584,6 +585,7 @@ static void test_sw_incr(void)
> > >  static void test_chained_counters(void)
> > >  {
> > >  	uint32_t events[] = {CPU_CYCLES, CHAIN};
> > > +	uint64_t cntr1;
> > >  
> > >  	if (!satisfy_prerequisites(events, ARRAY_SIZE(events)))
> > >  		return;
> > > @@ -618,13 +620,16 @@ static void test_chained_counters(void)
> > >  
> > >  	precise_instrs_loop(22, pmu.pmcr_ro | PMU_PMCR_E);
> > >  	report_info("overflow reg = 0x%lx", read_sysreg(pmovsclr_el0));
> > > -	report(!read_regn_el0(pmevcntr, 1), "CHAIN counter #1 wrapped");
> > > +	cntr1 = (pmu.version < ID_DFR0_PMU_V3_8_5) ? 0 : ALL_SET + 1;
> > > +	report(read_regn_el0(pmevcntr, 1) == cntr1, "CHAIN counter #1 wrapped");
> > 
> > It looks to me like the intention of the test was to check that the counter
> > programmed with the CHAIN event wraps, judging from the report message.
> > 
> 
> Ah, right. Yeah, that message is confusing. It should be the short
> version of "Inrementing at 32-bits resulted in the right value".
> 
> > I think it would be interesting to keep that by programming counter #1 with
> > ~0ULL when PMUv3p5 (maybe call it ALL_SET64?) and test the counter value
> > against 0.
> 
> The last commit adds tests using ALL_SET64.  Tests can be run in two
> modes: overflow_at_64bits and not.  However, this test,
> test_chained_counters(), and all other chained tests only use the
> !overflow_at_64bits mode (even after the last commit). The reason is
> that there are no CHAIN events when overflowing at 64-bits (more details
> in the commit message). But, don't worry, there are lots of tests that
> check wrapping at 64-bits (overflow_at_64bits=true).

What I was suggesting is this (change on top of this series, not on top of
this patch, to get access to ALL_SET_AT):

diff --git a/arm/pmu.c b/arm/pmu.c
index 3cb563b1abe4..fd7f20fc6c52 100644
--- a/arm/pmu.c
+++ b/arm/pmu.c
@@ -607,7 +607,6 @@ static void test_sw_incr(bool overflow_at_64bits)
 static void test_chained_counters(bool overflow_at_64bits)
 {
        uint32_t events[] = {CPU_CYCLES, CHAIN};
-       uint64_t cntr1;

        if (!satisfy_prerequisites(events, ARRAY_SIZE(events),
                                   overflow_at_64bits))
@@ -639,12 +638,11 @@ static void test_chained_counters(bool overflow_at_64bits)
        report(read_sysreg(pmovsclr_el0) == 0x1, "overflow recorded for chained incr #2");

        write_regn_el0(pmevcntr, 0, PRE_OVERFLOW);
-       write_regn_el0(pmevcntr, 1, ALL_SET);
+       write_regn_el0(pmevcntr, 1, ALL_SET_AT(overflow_at_64bits));

        precise_instrs_loop(22, pmu.pmcr_ro | PMU_PMCR_E);
        report_info("overflow reg = 0x%lx", read_sysreg(pmovsclr_el0));
-       cntr1 = (pmu.version < ID_DFR0_PMU_V3_8_5) ? 0 : ALL_SET + 1;
-       report(read_regn_el0(pmevcntr, 1) == cntr1, "CHAIN counter #1 wrapped");
+       report(read_regn_el0(pmevcntr, 1) == 0, "CHAIN counter #1 wrapped");

        report(read_sysreg(pmovsclr_el0) == 0x3, "overflow on even and odd counters");
 }

The counters are 64bit, but the CHAIN event should still work because
PMCR_EL0.LP is 0, according to the event description in the Arm ARM (ARM
DDI 0487I.a, page D17-6461).

Thanks,
Alex

> 
> > Alternatively, the report message can be modified, and "wrapped"
> > replaced with "incremented" (or something like that), to avoid confusion.
> > 
> > > +
> > >  	report(read_sysreg(pmovsclr_el0) == 0x3, "overflow on even and odd counters");
> > >  }
> > >  
> > >  static void test_chained_sw_incr(void)
> > >  {
> > >  	uint32_t events[] = {SW_INCR, CHAIN};
> > > +	uint64_t cntr0, cntr1;
> > >  	int i;
> > >  
> > >  	if (!satisfy_prerequisites(events, ARRAY_SIZE(events)))
> > > @@ -665,10 +670,12 @@ static void test_chained_sw_incr(void)
> > >  		write_sysreg(0x1, pmswinc_el0);
> > >  
> > >  	isb();
> > > +	cntr0 = (pmu.version < ID_DFR0_PMU_V3_8_5) ? 0 : ALL_SET + 1;
> > > +	cntr1 = (pmu.version < ID_DFR0_PMU_V3_8_5) ? 84 : PRE_OVERFLOW + 100;
> > >  	report((read_sysreg(pmovsclr_el0) == 0x3) &&
> > > -		(read_regn_el0(pmevcntr, 1) == 0) &&
> > > -		(read_regn_el0(pmevcntr, 0) == 84),
> > > -		"expected overflows and values after 100 SW_INCR/CHAIN");
> > > +	       (read_regn_el0(pmevcntr, 1) == cntr0) &&
> > > +	       (read_regn_el0(pmevcntr, 0) == cntr1),
> > 
> > This is hard to parse, it would be better if counter 0 is compared against
> > cntr0 and counter 1 against cntr1.
> 
> Ah, yes, code is correct but that's indeed confusing.
> 
> > 
> > Also, same suggestion here, looks like the test wants to check that the
> > odd-numbered counter wraps around when counting the CHAIN event.
> 
> Ack. Will update for v2.
> 
> Thanks!
> Ricardo
> 
> > 
> > Thanks,
> > Alex
> > 
> > > +	       "expected overflows and values after 100 SW_INCR/CHAIN");
> > >  	report_info("overflow=0x%lx, #0=%ld #1=%ld", read_sysreg(pmovsclr_el0),
> > >  		    read_regn_el0(pmevcntr, 0), read_regn_el0(pmevcntr, 1));
> > >  }
> > > -- 
> > > 2.39.0.rc0.267.gcb52ba06e7-goog
> > > 

Re: [kvm-unit-tests PATCH 1/3] arm: pmu: Fix overflow checks for PMUv3p5 long counters

[Ricardo Koller]

On Tue, Dec 13, 2022 at 04:43:38PM +0000, Alexandru Elisei wrote:
> Hi,
> 
> On Tue, Dec 13, 2022 at 08:21:24AM -0800, Ricardo Koller wrote:
> > On Tue, Dec 13, 2022 at 12:36:14PM +0000, Alexandru Elisei wrote:
> > > Hi,
> > > 
> > > Some more comments below.
> > > 
> > > On Fri, Dec 02, 2022 at 04:55:25AM +0000, Ricardo Koller wrote:
> > > > PMUv3p5 uses 64-bit counters irrespective of whether the PMU is configured
> > > > for overflowing at 32 or 64-bits. The consequence is that tests that check
> > > > the counter values after overflowing should not assume that values will be
> > > > wrapped around 32-bits: they overflow into the other half of the 64-bit
> > > > counters on PMUv3p5.
> > > > 
> > > > Fix tests by correctly checking overflowing-counters against the expected
> > > > 64-bit value.
> > > > 
> > > > Signed-off-by: Ricardo Koller <ricarkol@google.com>
> > > > ---
> > > >  arm/pmu.c | 29 ++++++++++++++++++-----------
> > > >  1 file changed, 18 insertions(+), 11 deletions(-)
> > > > 
> > > > diff --git a/arm/pmu.c b/arm/pmu.c
> > > > index cd47b14..eeac984 100644
> > > > --- a/arm/pmu.c
> > > > +++ b/arm/pmu.c
> > > > @@ -54,10 +54,10 @@
> > > >  #define EXT_COMMON_EVENTS_LOW	0x4000
> > > >  #define EXT_COMMON_EVENTS_HIGH	0x403F
> > > >  
> > > > -#define ALL_SET			0xFFFFFFFF
> > > > -#define ALL_CLEAR		0x0
> > > > -#define PRE_OVERFLOW		0xFFFFFFF0
> > > > -#define PRE_OVERFLOW2		0xFFFFFFDC
> > > > +#define ALL_SET			0x00000000FFFFFFFFULL
> > > > +#define ALL_CLEAR		0x0000000000000000ULL
> > > > +#define PRE_OVERFLOW		0x00000000FFFFFFF0ULL
> > > > +#define PRE_OVERFLOW2		0x00000000FFFFFFDCULL
> > > >  
> > > >  #define PMU_PPI			23
> > > >  
> > > > @@ -538,6 +538,7 @@ static void test_mem_access(void)
> > > >  static void test_sw_incr(void)
> > > >  {
> > > >  	uint32_t events[] = {SW_INCR, SW_INCR};
> > > > +	uint64_t cntr0;
> > > >  	int i;
> > > >  
> > > >  	if (!satisfy_prerequisites(events, ARRAY_SIZE(events)))
> > > > @@ -572,9 +573,9 @@ static void test_sw_incr(void)
> > > >  		write_sysreg(0x3, pmswinc_el0);
> > > >  
> > > >  	isb();
> > > > -	report(read_regn_el0(pmevcntr, 0)  == 84, "counter #1 after + 100 SW_INCR");
> > > > -	report(read_regn_el0(pmevcntr, 1)  == 100,
> > > > -		"counter #0 after + 100 SW_INCR");
> > > > +	cntr0 = (pmu.version < ID_DFR0_PMU_V3_8_5) ? 84 : PRE_OVERFLOW + 100;
> > > > +	report(read_regn_el0(pmevcntr, 0) == cntr0, "counter #0 after + 100 SW_INCR");
> > > > +	report(read_regn_el0(pmevcntr, 1) == 100, "counter #1 after + 100 SW_INCR");
> > > >  	report_info("counter values after 100 SW_INCR #0=%ld #1=%ld",
> > > >  		    read_regn_el0(pmevcntr, 0), read_regn_el0(pmevcntr, 1));
> > > >  	report(read_sysreg(pmovsclr_el0) == 0x1,
> > > > @@ -584,6 +585,7 @@ static void test_sw_incr(void)
> > > >  static void test_chained_counters(void)
> > > >  {
> > > >  	uint32_t events[] = {CPU_CYCLES, CHAIN};
> > > > +	uint64_t cntr1;
> > > >  
> > > >  	if (!satisfy_prerequisites(events, ARRAY_SIZE(events)))
> > > >  		return;
> > > > @@ -618,13 +620,16 @@ static void test_chained_counters(void)
> > > >  
> > > >  	precise_instrs_loop(22, pmu.pmcr_ro | PMU_PMCR_E);
> > > >  	report_info("overflow reg = 0x%lx", read_sysreg(pmovsclr_el0));
> > > > -	report(!read_regn_el0(pmevcntr, 1), "CHAIN counter #1 wrapped");
> > > > +	cntr1 = (pmu.version < ID_DFR0_PMU_V3_8_5) ? 0 : ALL_SET + 1;
> > > > +	report(read_regn_el0(pmevcntr, 1) == cntr1, "CHAIN counter #1 wrapped");
> > > 
> > > It looks to me like the intention of the test was to check that the counter
> > > programmed with the CHAIN event wraps, judging from the report message.
> > > 
> > 
> > Ah, right. Yeah, that message is confusing. It should be the short
> > version of "Inrementing at 32-bits resulted in the right value".
> > 
> > > I think it would be interesting to keep that by programming counter #1 with
> > > ~0ULL when PMUv3p5 (maybe call it ALL_SET64?) and test the counter value
> > > against 0.
> > 
> > The last commit adds tests using ALL_SET64.  Tests can be run in two
> > modes: overflow_at_64bits and not.  However, this test,
> > test_chained_counters(), and all other chained tests only use the
> > !overflow_at_64bits mode (even after the last commit). The reason is
> > that there are no CHAIN events when overflowing at 64-bits (more details
> > in the commit message). But, don't worry, there are lots of tests that
> > check wrapping at 64-bits (overflow_at_64bits=true).
> 
> What I was suggesting is this (change on top of this series, not on top of
> this patch, to get access to ALL_SET_AT):

Ooh, I see, I agree: it would be better to check that the odd counter
increments from ~0ULL to 0 when using 64-bit counters.

> 
> diff --git a/arm/pmu.c b/arm/pmu.c
> index 3cb563b1abe4..fd7f20fc6c52 100644
> --- a/arm/pmu.c
> +++ b/arm/pmu.c
> @@ -607,7 +607,6 @@ static void test_sw_incr(bool overflow_at_64bits)
>  static void test_chained_counters(bool overflow_at_64bits)
>  {
>         uint32_t events[] = {CPU_CYCLES, CHAIN};
> -       uint64_t cntr1;
> 
>         if (!satisfy_prerequisites(events, ARRAY_SIZE(events),
>                                    overflow_at_64bits))
> @@ -639,12 +638,11 @@ static void test_chained_counters(bool overflow_at_64bits)
>         report(read_sysreg(pmovsclr_el0) == 0x1, "overflow recorded for chained incr #2");
> 
>         write_regn_el0(pmevcntr, 0, PRE_OVERFLOW);
> -       write_regn_el0(pmevcntr, 1, ALL_SET);
> +       write_regn_el0(pmevcntr, 1, ALL_SET_AT(overflow_at_64bits));

The only change is that this should be:

	ALL_SET_AT(pmu.version >= ID_DFR0_PMU_V3_8_5)

Because "overflow_at_64bits" implies PMCR_EL0.LP = 1.

> 
>         precise_instrs_loop(22, pmu.pmcr_ro | PMU_PMCR_E);
>         report_info("overflow reg = 0x%lx", read_sysreg(pmovsclr_el0));
> -       cntr1 = (pmu.version < ID_DFR0_PMU_V3_8_5) ? 0 : ALL_SET + 1;
> -       report(read_regn_el0(pmevcntr, 1) == cntr1, "CHAIN counter #1 wrapped");
> +       report(read_regn_el0(pmevcntr, 1) == 0, "CHAIN counter #1 wrapped");
> 
>         report(read_sysreg(pmovsclr_el0) == 0x3, "overflow on even and odd counters");
>  }
> 
> The counters are 64bit, but the CHAIN event should still work because
> PMCR_EL0.LP is 0, according to the event description in the Arm ARM (ARM
> DDI 0487I.a, page D17-6461).
> 
> Thanks,
> Alex
> 
> > 
> > > Alternatively, the report message can be modified, and "wrapped"
> > > replaced with "incremented" (or something like that), to avoid confusion.
> > > 
> > > > +
> > > >  	report(read_sysreg(pmovsclr_el0) == 0x3, "overflow on even and odd counters");
> > > >  }
> > > >  
> > > >  static void test_chained_sw_incr(void)
> > > >  {
> > > >  	uint32_t events[] = {SW_INCR, CHAIN};
> > > > +	uint64_t cntr0, cntr1;
> > > >  	int i;
> > > >  
> > > >  	if (!satisfy_prerequisites(events, ARRAY_SIZE(events)))
> > > > @@ -665,10 +670,12 @@ static void test_chained_sw_incr(void)
> > > >  		write_sysreg(0x1, pmswinc_el0);
> > > >  
> > > >  	isb();
> > > > +	cntr0 = (pmu.version < ID_DFR0_PMU_V3_8_5) ? 0 : ALL_SET + 1;
> > > > +	cntr1 = (pmu.version < ID_DFR0_PMU_V3_8_5) ? 84 : PRE_OVERFLOW + 100;
> > > >  	report((read_sysreg(pmovsclr_el0) == 0x3) &&
> > > > -		(read_regn_el0(pmevcntr, 1) == 0) &&
> > > > -		(read_regn_el0(pmevcntr, 0) == 84),
> > > > -		"expected overflows and values after 100 SW_INCR/CHAIN");
> > > > +	       (read_regn_el0(pmevcntr, 1) == cntr0) &&
> > > > +	       (read_regn_el0(pmevcntr, 0) == cntr1),
> > > 
> > > This is hard to parse, it would be better if counter 0 is compared against
> > > cntr0 and counter 1 against cntr1.
> > 
> > Ah, yes, code is correct but that's indeed confusing.
> > 
> > > 
> > > Also, same suggestion here, looks like the test wants to check that the
> > > odd-numbered counter wraps around when counting the CHAIN event.
> > 
> > Ack. Will update for v2.
> > 
> > Thanks!
> > Ricardo
> > 
> > > 
> > > Thanks,
> > > Alex
> > > 
> > > > +	       "expected overflows and values after 100 SW_INCR/CHAIN");
> > > >  	report_info("overflow=0x%lx, #0=%ld #1=%ld", read_sysreg(pmovsclr_el0),
> > > >  		    read_regn_el0(pmevcntr, 0), read_regn_el0(pmevcntr, 1));
> > > >  }
> > > > -- 
> > > > 2.39.0.rc0.267.gcb52ba06e7-goog
> > > > 

Re: [kvm-unit-tests PATCH 1/3] arm: pmu: Fix overflow checks for PMUv3p5 long counters

[Alexandru Elisei]

Hi,

On Tue, Dec 13, 2022 at 10:01:06AM -0800, Ricardo Koller wrote:
> On Tue, Dec 13, 2022 at 04:43:38PM +0000, Alexandru Elisei wrote:
> > Hi,
> > 
> > On Tue, Dec 13, 2022 at 08:21:24AM -0800, Ricardo Koller wrote:
> > > On Tue, Dec 13, 2022 at 12:36:14PM +0000, Alexandru Elisei wrote:
> > > > Hi,
> > > > 
> > > > Some more comments below.
> > > > 
> > > > On Fri, Dec 02, 2022 at 04:55:25AM +0000, Ricardo Koller wrote:
> > > > > PMUv3p5 uses 64-bit counters irrespective of whether the PMU is configured
> > > > > for overflowing at 32 or 64-bits. The consequence is that tests that check
> > > > > the counter values after overflowing should not assume that values will be
> > > > > wrapped around 32-bits: they overflow into the other half of the 64-bit
> > > > > counters on PMUv3p5.
> > > > > 
> > > > > Fix tests by correctly checking overflowing-counters against the expected
> > > > > 64-bit value.
> > > > > 
> > > > > Signed-off-by: Ricardo Koller <ricarkol@google.com>
> > > > > ---
> > > > >  arm/pmu.c | 29 ++++++++++++++++++-----------
> > > > >  1 file changed, 18 insertions(+), 11 deletions(-)
> > > > > 
> > > > > diff --git a/arm/pmu.c b/arm/pmu.c
> > > > > index cd47b14..eeac984 100644
> > > > > --- a/arm/pmu.c
> > > > > +++ b/arm/pmu.c
> > > > > @@ -54,10 +54,10 @@
> > > > >  #define EXT_COMMON_EVENTS_LOW	0x4000
> > > > >  #define EXT_COMMON_EVENTS_HIGH	0x403F
> > > > >  
> > > > > -#define ALL_SET			0xFFFFFFFF
> > > > > -#define ALL_CLEAR		0x0
> > > > > -#define PRE_OVERFLOW		0xFFFFFFF0
> > > > > -#define PRE_OVERFLOW2		0xFFFFFFDC
> > > > > +#define ALL_SET			0x00000000FFFFFFFFULL
> > > > > +#define ALL_CLEAR		0x0000000000000000ULL
> > > > > +#define PRE_OVERFLOW		0x00000000FFFFFFF0ULL
> > > > > +#define PRE_OVERFLOW2		0x00000000FFFFFFDCULL
> > > > >  
> > > > >  #define PMU_PPI			23
> > > > >  
> > > > > @@ -538,6 +538,7 @@ static void test_mem_access(void)
> > > > >  static void test_sw_incr(void)
> > > > >  {
> > > > >  	uint32_t events[] = {SW_INCR, SW_INCR};
> > > > > +	uint64_t cntr0;
> > > > >  	int i;
> > > > >  
> > > > >  	if (!satisfy_prerequisites(events, ARRAY_SIZE(events)))
> > > > > @@ -572,9 +573,9 @@ static void test_sw_incr(void)
> > > > >  		write_sysreg(0x3, pmswinc_el0);
> > > > >  
> > > > >  	isb();
> > > > > -	report(read_regn_el0(pmevcntr, 0)  == 84, "counter #1 after + 100 SW_INCR");
> > > > > -	report(read_regn_el0(pmevcntr, 1)  == 100,
> > > > > -		"counter #0 after + 100 SW_INCR");
> > > > > +	cntr0 = (pmu.version < ID_DFR0_PMU_V3_8_5) ? 84 : PRE_OVERFLOW + 100;
> > > > > +	report(read_regn_el0(pmevcntr, 0) == cntr0, "counter #0 after + 100 SW_INCR");
> > > > > +	report(read_regn_el0(pmevcntr, 1) == 100, "counter #1 after + 100 SW_INCR");
> > > > >  	report_info("counter values after 100 SW_INCR #0=%ld #1=%ld",
> > > > >  		    read_regn_el0(pmevcntr, 0), read_regn_el0(pmevcntr, 1));
> > > > >  	report(read_sysreg(pmovsclr_el0) == 0x1,
> > > > > @@ -584,6 +585,7 @@ static void test_sw_incr(void)
> > > > >  static void test_chained_counters(void)
> > > > >  {
> > > > >  	uint32_t events[] = {CPU_CYCLES, CHAIN};
> > > > > +	uint64_t cntr1;
> > > > >  
> > > > >  	if (!satisfy_prerequisites(events, ARRAY_SIZE(events)))
> > > > >  		return;
> > > > > @@ -618,13 +620,16 @@ static void test_chained_counters(void)
> > > > >  
> > > > >  	precise_instrs_loop(22, pmu.pmcr_ro | PMU_PMCR_E);
> > > > >  	report_info("overflow reg = 0x%lx", read_sysreg(pmovsclr_el0));
> > > > > -	report(!read_regn_el0(pmevcntr, 1), "CHAIN counter #1 wrapped");
> > > > > +	cntr1 = (pmu.version < ID_DFR0_PMU_V3_8_5) ? 0 : ALL_SET + 1;
> > > > > +	report(read_regn_el0(pmevcntr, 1) == cntr1, "CHAIN counter #1 wrapped");
> > > > 
> > > > It looks to me like the intention of the test was to check that the counter
> > > > programmed with the CHAIN event wraps, judging from the report message.
> > > > 
> > > 
> > > Ah, right. Yeah, that message is confusing. It should be the short
> > > version of "Inrementing at 32-bits resulted in the right value".
> > > 
> > > > I think it would be interesting to keep that by programming counter #1 with
> > > > ~0ULL when PMUv3p5 (maybe call it ALL_SET64?) and test the counter value
> > > > against 0.
> > > 
> > > The last commit adds tests using ALL_SET64.  Tests can be run in two
> > > modes: overflow_at_64bits and not.  However, this test,
> > > test_chained_counters(), and all other chained tests only use the
> > > !overflow_at_64bits mode (even after the last commit). The reason is
> > > that there are no CHAIN events when overflowing at 64-bits (more details
> > > in the commit message). But, don't worry, there are lots of tests that
> > > check wrapping at 64-bits (overflow_at_64bits=true).
> > 
> > What I was suggesting is this (change on top of this series, not on top of
> > this patch, to get access to ALL_SET_AT):
> 
> Ooh, I see, I agree: it would be better to check that the odd counter
> increments from ~0ULL to 0 when using 64-bit counters.
> 
> > 
> > diff --git a/arm/pmu.c b/arm/pmu.c
> > index 3cb563b1abe4..fd7f20fc6c52 100644
> > --- a/arm/pmu.c
> > +++ b/arm/pmu.c
> > @@ -607,7 +607,6 @@ static void test_sw_incr(bool overflow_at_64bits)
> >  static void test_chained_counters(bool overflow_at_64bits)
> >  {
> >         uint32_t events[] = {CPU_CYCLES, CHAIN};
> > -       uint64_t cntr1;
> > 
> >         if (!satisfy_prerequisites(events, ARRAY_SIZE(events),
> >                                    overflow_at_64bits))
> > @@ -639,12 +638,11 @@ static void test_chained_counters(bool overflow_at_64bits)
> >         report(read_sysreg(pmovsclr_el0) == 0x1, "overflow recorded for chained incr #2");
> > 
> >         write_regn_el0(pmevcntr, 0, PRE_OVERFLOW);
> > -       write_regn_el0(pmevcntr, 1, ALL_SET);
> > +       write_regn_el0(pmevcntr, 1, ALL_SET_AT(overflow_at_64bits));
> 
> The only change is that this should be:
> 
> 	ALL_SET_AT(pmu.version >= ID_DFR0_PMU_V3_8_5)
> 
> Because "overflow_at_64bits" implies PMCR_EL0.LP = 1.

Right, and test_chained_counters() is never called with overflow_at_64bits
== true.

How about renaming the parameter overflow_at_64bits -> unused (or something
like that) to make it clear that the function does the same thing
regardless of the value?

Thanks,
Alex

> 
> > 
> >         precise_instrs_loop(22, pmu.pmcr_ro | PMU_PMCR_E);
> >         report_info("overflow reg = 0x%lx", read_sysreg(pmovsclr_el0));
> > -       cntr1 = (pmu.version < ID_DFR0_PMU_V3_8_5) ? 0 : ALL_SET + 1;
> > -       report(read_regn_el0(pmevcntr, 1) == cntr1, "CHAIN counter #1 wrapped");
> > +       report(read_regn_el0(pmevcntr, 1) == 0, "CHAIN counter #1 wrapped");
> > 
> >         report(read_sysreg(pmovsclr_el0) == 0x3, "overflow on even and odd counters");
> >  }
> > 
> > The counters are 64bit, but the CHAIN event should still work because
> > PMCR_EL0.LP is 0, according to the event description in the Arm ARM (ARM
> > DDI 0487I.a, page D17-6461).
> > 
> > Thanks,
> > Alex
> > 
> > > 
> > > > Alternatively, the report message can be modified, and "wrapped"
> > > > replaced with "incremented" (or something like that), to avoid confusion.
> > > > 
> > > > > +
> > > > >  	report(read_sysreg(pmovsclr_el0) == 0x3, "overflow on even and odd counters");
> > > > >  }
> > > > >  
> > > > >  static void test_chained_sw_incr(void)
> > > > >  {
> > > > >  	uint32_t events[] = {SW_INCR, CHAIN};
> > > > > +	uint64_t cntr0, cntr1;
> > > > >  	int i;
> > > > >  
> > > > >  	if (!satisfy_prerequisites(events, ARRAY_SIZE(events)))
> > > > > @@ -665,10 +670,12 @@ static void test_chained_sw_incr(void)
> > > > >  		write_sysreg(0x1, pmswinc_el0);
> > > > >  
> > > > >  	isb();
> > > > > +	cntr0 = (pmu.version < ID_DFR0_PMU_V3_8_5) ? 0 : ALL_SET + 1;
> > > > > +	cntr1 = (pmu.version < ID_DFR0_PMU_V3_8_5) ? 84 : PRE_OVERFLOW + 100;
> > > > >  	report((read_sysreg(pmovsclr_el0) == 0x3) &&
> > > > > -		(read_regn_el0(pmevcntr, 1) == 0) &&
> > > > > -		(read_regn_el0(pmevcntr, 0) == 84),
> > > > > -		"expected overflows and values after 100 SW_INCR/CHAIN");
> > > > > +	       (read_regn_el0(pmevcntr, 1) == cntr0) &&
> > > > > +	       (read_regn_el0(pmevcntr, 0) == cntr1),
> > > > 
> > > > This is hard to parse, it would be better if counter 0 is compared against
> > > > cntr0 and counter 1 against cntr1.
> > > 
> > > Ah, yes, code is correct but that's indeed confusing.
> > > 
> > > > 
> > > > Also, same suggestion here, looks like the test wants to check that the
> > > > odd-numbered counter wraps around when counting the CHAIN event.
> > > 
> > > Ack. Will update for v2.
> > > 
> > > Thanks!
> > > Ricardo
> > > 
> > > > 
> > > > Thanks,
> > > > Alex
> > > > 
> > > > > +	       "expected overflows and values after 100 SW_INCR/CHAIN");
> > > > >  	report_info("overflow=0x%lx, #0=%ld #1=%ld", read_sysreg(pmovsclr_el0),
> > > > >  		    read_regn_el0(pmevcntr, 0), read_regn_el0(pmevcntr, 1));
> > > > >  }
> > > > > -- 
> > > > > 2.39.0.rc0.267.gcb52ba06e7-goog
> > > > > 

Re: [kvm-unit-tests PATCH 1/3] arm: pmu: Fix overflow checks for PMUv3p5 long counters

[Ricardo Koller]

On Wed, Dec 14, 2022 at 10:46:05AM +0000, Alexandru Elisei wrote:
> Hi,
> 
> On Tue, Dec 13, 2022 at 10:01:06AM -0800, Ricardo Koller wrote:
> > On Tue, Dec 13, 2022 at 04:43:38PM +0000, Alexandru Elisei wrote:
> > > Hi,
> > > 
> > > On Tue, Dec 13, 2022 at 08:21:24AM -0800, Ricardo Koller wrote:
> > > > On Tue, Dec 13, 2022 at 12:36:14PM +0000, Alexandru Elisei wrote:
> > > > > Hi,
> > > > > 
> > > > > Some more comments below.
> > > > > 
> > > > > On Fri, Dec 02, 2022 at 04:55:25AM +0000, Ricardo Koller wrote:
> > > > > > PMUv3p5 uses 64-bit counters irrespective of whether the PMU is configured
> > > > > > for overflowing at 32 or 64-bits. The consequence is that tests that check
> > > > > > the counter values after overflowing should not assume that values will be
> > > > > > wrapped around 32-bits: they overflow into the other half of the 64-bit
> > > > > > counters on PMUv3p5.
> > > > > > 
> > > > > > Fix tests by correctly checking overflowing-counters against the expected
> > > > > > 64-bit value.
> > > > > > 
> > > > > > Signed-off-by: Ricardo Koller <ricarkol@google.com>
> > > > > > ---
> > > > > >  arm/pmu.c | 29 ++++++++++++++++++-----------
> > > > > >  1 file changed, 18 insertions(+), 11 deletions(-)
> > > > > > 
> > > > > > diff --git a/arm/pmu.c b/arm/pmu.c
> > > > > > index cd47b14..eeac984 100644
> > > > > > --- a/arm/pmu.c
> > > > > > +++ b/arm/pmu.c
> > > > > > @@ -54,10 +54,10 @@
> > > > > >  #define EXT_COMMON_EVENTS_LOW	0x4000
> > > > > >  #define EXT_COMMON_EVENTS_HIGH	0x403F
> > > > > >  
> > > > > > -#define ALL_SET			0xFFFFFFFF
> > > > > > -#define ALL_CLEAR		0x0
> > > > > > -#define PRE_OVERFLOW		0xFFFFFFF0
> > > > > > -#define PRE_OVERFLOW2		0xFFFFFFDC
> > > > > > +#define ALL_SET			0x00000000FFFFFFFFULL
> > > > > > +#define ALL_CLEAR		0x0000000000000000ULL
> > > > > > +#define PRE_OVERFLOW		0x00000000FFFFFFF0ULL
> > > > > > +#define PRE_OVERFLOW2		0x00000000FFFFFFDCULL
> > > > > >  
> > > > > >  #define PMU_PPI			23
> > > > > >  
> > > > > > @@ -538,6 +538,7 @@ static void test_mem_access(void)
> > > > > >  static void test_sw_incr(void)
> > > > > >  {
> > > > > >  	uint32_t events[] = {SW_INCR, SW_INCR};
> > > > > > +	uint64_t cntr0;
> > > > > >  	int i;
> > > > > >  
> > > > > >  	if (!satisfy_prerequisites(events, ARRAY_SIZE(events)))
> > > > > > @@ -572,9 +573,9 @@ static void test_sw_incr(void)
> > > > > >  		write_sysreg(0x3, pmswinc_el0);
> > > > > >  
> > > > > >  	isb();
> > > > > > -	report(read_regn_el0(pmevcntr, 0)  == 84, "counter #1 after + 100 SW_INCR");
> > > > > > -	report(read_regn_el0(pmevcntr, 1)  == 100,
> > > > > > -		"counter #0 after + 100 SW_INCR");
> > > > > > +	cntr0 = (pmu.version < ID_DFR0_PMU_V3_8_5) ? 84 : PRE_OVERFLOW + 100;
> > > > > > +	report(read_regn_el0(pmevcntr, 0) == cntr0, "counter #0 after + 100 SW_INCR");
> > > > > > +	report(read_regn_el0(pmevcntr, 1) == 100, "counter #1 after + 100 SW_INCR");
> > > > > >  	report_info("counter values after 100 SW_INCR #0=%ld #1=%ld",
> > > > > >  		    read_regn_el0(pmevcntr, 0), read_regn_el0(pmevcntr, 1));
> > > > > >  	report(read_sysreg(pmovsclr_el0) == 0x1,
> > > > > > @@ -584,6 +585,7 @@ static void test_sw_incr(void)
> > > > > >  static void test_chained_counters(void)
> > > > > >  {
> > > > > >  	uint32_t events[] = {CPU_CYCLES, CHAIN};
> > > > > > +	uint64_t cntr1;
> > > > > >  
> > > > > >  	if (!satisfy_prerequisites(events, ARRAY_SIZE(events)))
> > > > > >  		return;
> > > > > > @@ -618,13 +620,16 @@ static void test_chained_counters(void)
> > > > > >  
> > > > > >  	precise_instrs_loop(22, pmu.pmcr_ro | PMU_PMCR_E);
> > > > > >  	report_info("overflow reg = 0x%lx", read_sysreg(pmovsclr_el0));
> > > > > > -	report(!read_regn_el0(pmevcntr, 1), "CHAIN counter #1 wrapped");
> > > > > > +	cntr1 = (pmu.version < ID_DFR0_PMU_V3_8_5) ? 0 : ALL_SET + 1;
> > > > > > +	report(read_regn_el0(pmevcntr, 1) == cntr1, "CHAIN counter #1 wrapped");
> > > > > 
> > > > > It looks to me like the intention of the test was to check that the counter
> > > > > programmed with the CHAIN event wraps, judging from the report message.
> > > > > 
> > > > 
> > > > Ah, right. Yeah, that message is confusing. It should be the short
> > > > version of "Inrementing at 32-bits resulted in the right value".
> > > > 
> > > > > I think it would be interesting to keep that by programming counter #1 with
> > > > > ~0ULL when PMUv3p5 (maybe call it ALL_SET64?) and test the counter value
> > > > > against 0.
> > > > 
> > > > The last commit adds tests using ALL_SET64.  Tests can be run in two
> > > > modes: overflow_at_64bits and not.  However, this test,
> > > > test_chained_counters(), and all other chained tests only use the
> > > > !overflow_at_64bits mode (even after the last commit). The reason is
> > > > that there are no CHAIN events when overflowing at 64-bits (more details
> > > > in the commit message). But, don't worry, there are lots of tests that
> > > > check wrapping at 64-bits (overflow_at_64bits=true).
> > > 
> > > What I was suggesting is this (change on top of this series, not on top of
> > > this patch, to get access to ALL_SET_AT):
> > 
> > Ooh, I see, I agree: it would be better to check that the odd counter
> > increments from ~0ULL to 0 when using 64-bit counters.
> > 
> > > 
> > > diff --git a/arm/pmu.c b/arm/pmu.c
> > > index 3cb563b1abe4..fd7f20fc6c52 100644
> > > --- a/arm/pmu.c
> > > +++ b/arm/pmu.c
> > > @@ -607,7 +607,6 @@ static void test_sw_incr(bool overflow_at_64bits)
> > >  static void test_chained_counters(bool overflow_at_64bits)
> > >  {
> > >         uint32_t events[] = {CPU_CYCLES, CHAIN};
> > > -       uint64_t cntr1;
> > > 
> > >         if (!satisfy_prerequisites(events, ARRAY_SIZE(events),
> > >                                    overflow_at_64bits))
> > > @@ -639,12 +638,11 @@ static void test_chained_counters(bool overflow_at_64bits)
> > >         report(read_sysreg(pmovsclr_el0) == 0x1, "overflow recorded for chained incr #2");
> > > 
> > >         write_regn_el0(pmevcntr, 0, PRE_OVERFLOW);
> > > -       write_regn_el0(pmevcntr, 1, ALL_SET);
> > > +       write_regn_el0(pmevcntr, 1, ALL_SET_AT(overflow_at_64bits));
> > 
> > The only change is that this should be:
> > 
> > 	ALL_SET_AT(pmu.version >= ID_DFR0_PMU_V3_8_5)
> > 
> > Because "overflow_at_64bits" implies PMCR_EL0.LP = 1.
> 
> Right, and test_chained_counters() is never called with overflow_at_64bits
> == true.
> 
> How about renaming the parameter overflow_at_64bits -> unused (or something
> like that) to make it clear that the function does the same thing
> regardless of the value?

Sounds good, will do for v2.

> 
> Thanks,
> Alex
> 
> > 
> > > 
> > >         precise_instrs_loop(22, pmu.pmcr_ro | PMU_PMCR_E);
> > >         report_info("overflow reg = 0x%lx", read_sysreg(pmovsclr_el0));
> > > -       cntr1 = (pmu.version < ID_DFR0_PMU_V3_8_5) ? 0 : ALL_SET + 1;
> > > -       report(read_regn_el0(pmevcntr, 1) == cntr1, "CHAIN counter #1 wrapped");
> > > +       report(read_regn_el0(pmevcntr, 1) == 0, "CHAIN counter #1 wrapped");
> > > 
> > >         report(read_sysreg(pmovsclr_el0) == 0x3, "overflow on even and odd counters");
> > >  }
> > > 
> > > The counters are 64bit, but the CHAIN event should still work because
> > > PMCR_EL0.LP is 0, according to the event description in the Arm ARM (ARM
> > > DDI 0487I.a, page D17-6461).
> > > 
> > > Thanks,
> > > Alex
> > > 
> > > > 
> > > > > Alternatively, the report message can be modified, and "wrapped"
> > > > > replaced with "incremented" (or something like that), to avoid confusion.
> > > > > 
> > > > > > +
> > > > > >  	report(read_sysreg(pmovsclr_el0) == 0x3, "overflow on even and odd counters");
> > > > > >  }
> > > > > >  
> > > > > >  static void test_chained_sw_incr(void)
> > > > > >  {
> > > > > >  	uint32_t events[] = {SW_INCR, CHAIN};
> > > > > > +	uint64_t cntr0, cntr1;
> > > > > >  	int i;
> > > > > >  
> > > > > >  	if (!satisfy_prerequisites(events, ARRAY_SIZE(events)))
> > > > > > @@ -665,10 +670,12 @@ static void test_chained_sw_incr(void)
> > > > > >  		write_sysreg(0x1, pmswinc_el0);
> > > > > >  
> > > > > >  	isb();
> > > > > > +	cntr0 = (pmu.version < ID_DFR0_PMU_V3_8_5) ? 0 : ALL_SET + 1;
> > > > > > +	cntr1 = (pmu.version < ID_DFR0_PMU_V3_8_5) ? 84 : PRE_OVERFLOW + 100;
> > > > > >  	report((read_sysreg(pmovsclr_el0) == 0x3) &&
> > > > > > -		(read_regn_el0(pmevcntr, 1) == 0) &&
> > > > > > -		(read_regn_el0(pmevcntr, 0) == 84),
> > > > > > -		"expected overflows and values after 100 SW_INCR/CHAIN");
> > > > > > +	       (read_regn_el0(pmevcntr, 1) == cntr0) &&
> > > > > > +	       (read_regn_el0(pmevcntr, 0) == cntr1),
> > > > > 
> > > > > This is hard to parse, it would be better if counter 0 is compared against
> > > > > cntr0 and counter 1 against cntr1.
> > > > 
> > > > Ah, yes, code is correct but that's indeed confusing.
> > > > 
> > > > > 
> > > > > Also, same suggestion here, looks like the test wants to check that the
> > > > > odd-numbered counter wraps around when counting the CHAIN event.
> > > > 
> > > > Ack. Will update for v2.
> > > > 
> > > > Thanks!
> > > > Ricardo
> > > > 
> > > > > 
> > > > > Thanks,
> > > > > Alex
> > > > > 
> > > > > > +	       "expected overflows and values after 100 SW_INCR/CHAIN");
> > > > > >  	report_info("overflow=0x%lx, #0=%ld #1=%ld", read_sysreg(pmovsclr_el0),
> > > > > >  		    read_regn_el0(pmevcntr, 0), read_regn_el0(pmevcntr, 1));
> > > > > >  }
> > > > > > -- 
> > > > > > 2.39.0.rc0.267.gcb52ba06e7-goog
> > > > > > 

[kvm-unit-tests PATCH 2/3] arm: pmu: Prepare for testing 64-bit overflows

[Ricardo Koller]

PMUv3p5 adds a knob, PMCR_EL0.LP == 1, that allows overflowing at 64-bits
instead of 32. Prepare by doing these 3 things:

1. Add a "bool overflow_at_64bits" argument to all tests checking
   overflows.
2. Extend satisfy_prerequisites() to check if the machine supports
   "overflow_at_64bits".
3. Refactor the test invocations to use the new "run_test()" which adds a
   report prefix indicating whether the test uses 64 or 32-bit overflows.

A subsequent commit will actually add the 64-bit overflow tests.

Signed-off-by: Ricardo Koller <ricarkol@google.com>
---
 arm/pmu.c | 99 +++++++++++++++++++++++++++++++------------------------
 1 file changed, 56 insertions(+), 43 deletions(-)

diff --git a/arm/pmu.c b/arm/pmu.c
index eeac984..59e5bfe 100644
--- a/arm/pmu.c
+++ b/arm/pmu.c
@@ -164,13 +164,13 @@ static void pmu_reset(void)
 /* 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) {}
-static void test_sw_incr(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) {}
+static void test_basic_event_count(bool overflow_at_64bits) {}
+static void test_mem_access(bool overflow_at_64bits) {}
+static void test_sw_incr(bool overflow_at_64bits) {}
+static void test_chained_counters(bool overflow_at_64bits) {}
+static void test_chained_sw_incr(bool overflow_at_64bits) {}
+static void test_chain_promotion(bool overflow_at_64bits) {}
+static void test_overflow_interrupt(bool overflow_at_64bits) {}
 
 #elif defined(__aarch64__)
 #define ID_AA64DFR0_PERFMON_SHIFT 8
@@ -399,7 +399,8 @@ static void test_event_counter_config(void)
 		"read of a counter programmed with unsupported event");
 }
 
-static bool satisfy_prerequisites(uint32_t *events, unsigned int nb_events)
+static bool satisfy_prerequisites(uint32_t *events, unsigned int nb_events,
+				  bool overflow_at_64bits)
 {
 	int i;
 
@@ -416,16 +417,23 @@ static bool satisfy_prerequisites(uint32_t *events, unsigned int nb_events)
 			return false;
 		}
 	}
+
+	if (overflow_at_64bits && pmu.version < ID_DFR0_PMU_V3_8_5) {
+		report_skip("Skip test as 64 overflows need FEAT_PMUv3p5");
+		return false;
+	}
+
 	return true;
 }
 
-static void test_basic_event_count(void)
+static void test_basic_event_count(bool overflow_at_64bits)
 {
 	uint32_t implemented_counter_mask, non_implemented_counter_mask;
 	uint32_t counter_mask;
 	uint32_t events[] = {CPU_CYCLES, INST_RETIRED};
 
-	if (!satisfy_prerequisites(events, ARRAY_SIZE(events)))
+	if (!satisfy_prerequisites(events, ARRAY_SIZE(events),
+				   overflow_at_64bits))
 		return;
 
 	implemented_counter_mask = BIT(pmu.nb_implemented_counters) - 1;
@@ -499,12 +507,13 @@ static void test_basic_event_count(void)
 		"check overflow happened on #0 only");
 }
 
-static void test_mem_access(void)
+static void test_mem_access(bool overflow_at_64bits)
 {
 	void *addr = malloc(PAGE_SIZE);
 	uint32_t events[] = {MEM_ACCESS, MEM_ACCESS};
 
-	if (!satisfy_prerequisites(events, ARRAY_SIZE(events)))
+	if (!satisfy_prerequisites(events, ARRAY_SIZE(events),
+				   overflow_at_64bits))
 		return;
 
 	pmu_reset();
@@ -535,13 +544,14 @@ static void test_mem_access(void)
 			read_sysreg(pmovsclr_el0));
 }
 
-static void test_sw_incr(void)
+static void test_sw_incr(bool overflow_at_64bits)
 {
 	uint32_t events[] = {SW_INCR, SW_INCR};
 	uint64_t cntr0;
 	int i;
 
-	if (!satisfy_prerequisites(events, ARRAY_SIZE(events)))
+	if (!satisfy_prerequisites(events, ARRAY_SIZE(events),
+				   overflow_at_64bits))
 		return;
 
 	pmu_reset();
@@ -582,12 +592,13 @@ static void test_sw_incr(void)
 		"overflow on counter #0 after 100 SW_INCR");
 }
 
-static void test_chained_counters(void)
+static void test_chained_counters(bool overflow_at_64bits)
 {
 	uint32_t events[] = {CPU_CYCLES, CHAIN};
 	uint64_t cntr1;
 
-	if (!satisfy_prerequisites(events, ARRAY_SIZE(events)))
+	if (!satisfy_prerequisites(events, ARRAY_SIZE(events),
+				   overflow_at_64bits))
 		return;
 
 	pmu_reset();
@@ -626,13 +637,14 @@ static void test_chained_counters(void)
 	report(read_sysreg(pmovsclr_el0) == 0x3, "overflow on even and odd counters");
 }
 
-static void test_chained_sw_incr(void)
+static void test_chained_sw_incr(bool overflow_at_64bits)
 {
 	uint32_t events[] = {SW_INCR, CHAIN};
 	uint64_t cntr0, cntr1;
 	int i;
 
-	if (!satisfy_prerequisites(events, ARRAY_SIZE(events)))
+	if (!satisfy_prerequisites(events, ARRAY_SIZE(events),
+				   overflow_at_64bits))
 		return;
 
 	pmu_reset();
@@ -680,12 +692,13 @@ static void test_chained_sw_incr(void)
 		    read_regn_el0(pmevcntr, 0), read_regn_el0(pmevcntr, 1));
 }
 
-static void test_chain_promotion(void)
+static void test_chain_promotion(bool overflow_at_64bits)
 {
 	uint32_t events[] = {MEM_ACCESS, CHAIN};
 	void *addr = malloc(PAGE_SIZE);
 
-	if (!satisfy_prerequisites(events, ARRAY_SIZE(events)))
+	if (!satisfy_prerequisites(events, ARRAY_SIZE(events),
+				   overflow_at_64bits))
 		return;
 
 	/* Only enable CHAIN counter */
@@ -829,13 +842,14 @@ static bool expect_interrupts(uint32_t bitmap)
 	return true;
 }
 
-static void test_overflow_interrupt(void)
+static void test_overflow_interrupt(bool overflow_at_64bits)
 {
 	uint32_t events[] = {MEM_ACCESS, SW_INCR};
 	void *addr = malloc(PAGE_SIZE);
 	int i;
 
-	if (!satisfy_prerequisites(events, ARRAY_SIZE(events)))
+	if (!satisfy_prerequisites(events, ARRAY_SIZE(events),
+				   overflow_at_64bits))
 		return;
 
 	gic_enable_defaults();
@@ -1059,6 +1073,19 @@ static bool pmu_probe(void)
 	return true;
 }
 
+static void run_test(char *name, void (*test)(bool), bool overflow_at_64bits)
+{
+	const char *prefix = overflow_at_64bits ? "64-bit" : "32-bit";
+
+	report_prefix_push(name);
+	report_prefix_push(prefix);
+
+	test(overflow_at_64bits);
+
+	report_prefix_pop();
+	report_prefix_pop();
+}
+
 int main(int argc, char *argv[])
 {
 	int cpi = 0;
@@ -1091,33 +1118,19 @@ int main(int argc, char *argv[])
 		test_event_counter_config();
 		report_prefix_pop();
 	} else if (strcmp(argv[1], "pmu-basic-event-count") == 0) {
-		report_prefix_push(argv[1]);
-		test_basic_event_count();
-		report_prefix_pop();
+		run_test(argv[1], test_basic_event_count, false);
 	} else if (strcmp(argv[1], "pmu-mem-access") == 0) {
-		report_prefix_push(argv[1]);
-		test_mem_access();
-		report_prefix_pop();
+		run_test(argv[1], test_mem_access, false);
 	} else if (strcmp(argv[1], "pmu-sw-incr") == 0) {
-		report_prefix_push(argv[1]);
-		test_sw_incr();
-		report_prefix_pop();
+		run_test(argv[1], test_sw_incr, false);
 	} else if (strcmp(argv[1], "pmu-chained-counters") == 0) {
-		report_prefix_push(argv[1]);
-		test_chained_counters();
-		report_prefix_pop();
+		run_test(argv[1], test_chained_counters, false);
 	} else if (strcmp(argv[1], "pmu-chained-sw-incr") == 0) {
-		report_prefix_push(argv[1]);
-		test_chained_sw_incr();
-		report_prefix_pop();
+		run_test(argv[1], test_chained_sw_incr, false);
 	} else if (strcmp(argv[1], "pmu-chain-promotion") == 0) {
-		report_prefix_push(argv[1]);
-		test_chain_promotion();
-		report_prefix_pop();
+		run_test(argv[1], test_chain_promotion, false);
 	} else if (strcmp(argv[1], "pmu-overflow-interrupt") == 0) {
-		report_prefix_push(argv[1]);
-		test_overflow_interrupt();
-		report_prefix_pop();
+		run_test(argv[1], test_overflow_interrupt, false);
 	} else {
 		report_abort("Unknown sub-test '%s'", argv[1]);
 	}
-- 
2.39.0.rc0.267.gcb52ba06e7-goog

[kvm-unit-tests PATCH 3/3] arm: pmu: Add tests for 64-bit overflows

[Ricardo Koller]

Modify all tests checking overflows to support both 32 (PMCR_EL0.LP == 0)
and 64-bit overflows (PMCR_EL0.LP == 1). 64-bit overflows are only
supported on PMUv3p5.

Note that chained tests do not implement "overflow_at_64bits == true".
That's because there are no CHAIN events when "PMCR_EL0.LP == 1" (for more
details see AArch64.IncrementEventCounter() pseudocode in the ARM ARM DDI
0487H.a, J1.1.1 "aarch64/debug").

Signed-off-by: Ricardo Koller <ricarkol@google.com>
---
 arm/pmu.c | 91 ++++++++++++++++++++++++++++++++++++-------------------
 1 file changed, 60 insertions(+), 31 deletions(-)

diff --git a/arm/pmu.c b/arm/pmu.c
index 59e5bfe..3cb563b 100644
--- a/arm/pmu.c
+++ b/arm/pmu.c
@@ -28,6 +28,7 @@
 #define PMU_PMCR_X         (1 << 4)
 #define PMU_PMCR_DP        (1 << 5)
 #define PMU_PMCR_LC        (1 << 6)
+#define PMU_PMCR_LP        (1 << 7)
 #define PMU_PMCR_N_SHIFT   11
 #define PMU_PMCR_N_MASK    0x1f
 #define PMU_PMCR_ID_SHIFT  16
@@ -55,10 +56,15 @@
 #define EXT_COMMON_EVENTS_HIGH	0x403F
 
 #define ALL_SET			0x00000000FFFFFFFFULL
+#define ALL_SET_64		0xFFFFFFFFFFFFFFFFULL
 #define ALL_CLEAR		0x0000000000000000ULL
 #define PRE_OVERFLOW		0x00000000FFFFFFF0ULL
+#define PRE_OVERFLOW_64		0xFFFFFFFFFFFFFFF0ULL
 #define PRE_OVERFLOW2		0x00000000FFFFFFDCULL
 
+#define PRE_OVERFLOW_AT(_64b)	(_64b ? PRE_OVERFLOW_64 : PRE_OVERFLOW)
+#define ALL_SET_AT(_64b)	(_64b ? ALL_SET_64 : ALL_SET)
+
 #define PMU_PPI			23
 
 struct pmu {
@@ -429,8 +435,10 @@ static bool satisfy_prerequisites(uint32_t *events, unsigned int nb_events,
 static void test_basic_event_count(bool overflow_at_64bits)
 {
 	uint32_t implemented_counter_mask, non_implemented_counter_mask;
-	uint32_t counter_mask;
+	uint64_t pre_overflow = PRE_OVERFLOW_AT(overflow_at_64bits);
+	uint64_t pmcr_lp = overflow_at_64bits ? PMU_PMCR_LP : 0;
 	uint32_t events[] = {CPU_CYCLES, INST_RETIRED};
+	uint32_t counter_mask;
 
 	if (!satisfy_prerequisites(events, ARRAY_SIZE(events),
 				   overflow_at_64bits))
@@ -452,13 +460,13 @@ static void test_basic_event_count(bool overflow_at_64bits)
 	 * 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);
+	set_pmcr(pmu.pmcr_ro | PMU_PMCR_LC | PMU_PMCR_C | PMU_PMCR_P | pmcr_lp);
 	isb();
-	report(get_pmcr() == (pmu.pmcr_ro | PMU_PMCR_LC), "pmcr: reset counters");
+	report(get_pmcr() == (pmu.pmcr_ro | PMU_PMCR_LC | pmcr_lp), "pmcr: reset counters");
 
 	/* Preset counter #0 to pre overflow value to trigger an overflow */
-	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW);
-	report(read_regn_el0(pmevcntr, 0) == PRE_OVERFLOW,
+	write_regn_el0(pmevcntr, 0, pre_overflow);
+	report(read_regn_el0(pmevcntr, 0) == pre_overflow,
 		"counter #0 preset to pre-overflow value");
 	report(!read_regn_el0(pmevcntr, 1), "counter #1 is 0");
 
@@ -511,6 +519,8 @@ static void test_mem_access(bool overflow_at_64bits)
 {
 	void *addr = malloc(PAGE_SIZE);
 	uint32_t events[] = {MEM_ACCESS, MEM_ACCESS};
+	uint64_t pre_overflow = PRE_OVERFLOW_AT(overflow_at_64bits);
+	uint64_t pmcr_lp = overflow_at_64bits ? PMU_PMCR_LP : 0;
 
 	if (!satisfy_prerequisites(events, ARRAY_SIZE(events),
 				   overflow_at_64bits))
@@ -522,7 +532,7 @@ static void test_mem_access(bool overflow_at_64bits)
 	write_regn_el0(pmevtyper, 1, MEM_ACCESS | PMEVTYPER_EXCLUDE_EL0);
 	write_sysreg_s(0x3, PMCNTENSET_EL0);
 	isb();
-	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E);
+	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E | pmcr_lp);
 	report_info("counter #0 is %ld (MEM_ACCESS)", read_regn_el0(pmevcntr, 0));
 	report_info("counter #1 is %ld (MEM_ACCESS)", read_regn_el0(pmevcntr, 1));
 	/* We may measure more than 20 mem access depending on the core */
@@ -532,11 +542,11 @@ static void test_mem_access(bool overflow_at_64bits)
 
 	pmu_reset();
 
-	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW);
-	write_regn_el0(pmevcntr, 1, PRE_OVERFLOW);
+	write_regn_el0(pmevcntr, 0, pre_overflow);
+	write_regn_el0(pmevcntr, 1, pre_overflow);
 	write_sysreg_s(0x3, PMCNTENSET_EL0);
 	isb();
-	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E);
+	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E | pmcr_lp);
 	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",
@@ -546,6 +556,8 @@ static void test_mem_access(bool overflow_at_64bits)
 
 static void test_sw_incr(bool overflow_at_64bits)
 {
+	uint64_t pre_overflow = PRE_OVERFLOW_AT(overflow_at_64bits);
+	uint64_t pmcr_lp = overflow_at_64bits ? PMU_PMCR_LP : 0;
 	uint32_t events[] = {SW_INCR, SW_INCR};
 	uint64_t cntr0;
 	int i;
@@ -561,7 +573,7 @@ static void test_sw_incr(bool overflow_at_64bits)
 	/* enable counters #0 and #1 */
 	write_sysreg_s(0x3, PMCNTENSET_EL0);
 
-	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW);
+	write_regn_el0(pmevcntr, 0, pre_overflow);
 	isb();
 
 	for (i = 0; i < 100; i++)
@@ -569,21 +581,21 @@ static void test_sw_incr(bool overflow_at_64bits)
 
 	isb();
 	report_info("SW_INCR counter #0 has value %ld", read_regn_el0(pmevcntr, 0));
-	report(read_regn_el0(pmevcntr, 0) == PRE_OVERFLOW,
+	report(read_regn_el0(pmevcntr, 0) == pre_overflow,
 		"PWSYNC does not increment if PMCR.E is unset");
 
 	pmu_reset();
 
-	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW);
+	write_regn_el0(pmevcntr, 0, pre_overflow);
 	write_sysreg_s(0x3, PMCNTENSET_EL0);
-	set_pmcr(pmu.pmcr_ro | PMU_PMCR_E);
+	set_pmcr(pmu.pmcr_ro | PMU_PMCR_E | pmcr_lp);
 	isb();
 
 	for (i = 0; i < 100; i++)
 		write_sysreg(0x3, pmswinc_el0);
 
 	isb();
-	cntr0 = (pmu.version < ID_DFR0_PMU_V3_8_5) ? 84 : PRE_OVERFLOW + 100;
+	cntr0 = (pmu.version < ID_DFR0_PMU_V3_8_5) ? 84 : pre_overflow + 100;
 	report(read_regn_el0(pmevcntr, 0) == cntr0, "counter #0 after + 100 SW_INCR");
 	report(read_regn_el0(pmevcntr, 1) == 100, "counter #1 after + 100 SW_INCR");
 	report_info("counter values after 100 SW_INCR #0=%ld #1=%ld",
@@ -844,6 +856,9 @@ static bool expect_interrupts(uint32_t bitmap)
 
 static void test_overflow_interrupt(bool overflow_at_64bits)
 {
+	uint64_t pre_overflow = PRE_OVERFLOW_AT(overflow_at_64bits);
+	uint64_t all_set = ALL_SET_AT(overflow_at_64bits);
+	uint64_t pmcr_lp = overflow_at_64bits ? PMU_PMCR_LP : 0;
 	uint32_t events[] = {MEM_ACCESS, SW_INCR};
 	void *addr = malloc(PAGE_SIZE);
 	int i;
@@ -862,16 +877,16 @@ static void test_overflow_interrupt(bool overflow_at_64bits)
 	write_regn_el0(pmevtyper, 0, MEM_ACCESS | PMEVTYPER_EXCLUDE_EL0);
 	write_regn_el0(pmevtyper, 1, SW_INCR | PMEVTYPER_EXCLUDE_EL0);
 	write_sysreg_s(0x3, PMCNTENSET_EL0);
-	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW);
-	write_regn_el0(pmevcntr, 1, PRE_OVERFLOW);
+	write_regn_el0(pmevcntr, 0, pre_overflow);
+	write_regn_el0(pmevcntr, 1, pre_overflow);
 	isb();
 
 	/* interrupts are disabled (PMINTENSET_EL1 == 0) */
 
-	mem_access_loop(addr, 200, pmu.pmcr_ro | PMU_PMCR_E);
+	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E | pmcr_lp);
 	report(expect_interrupts(0), "no overflow interrupt after preset");
 
-	set_pmcr(pmu.pmcr_ro | PMU_PMCR_E);
+	set_pmcr(pmu.pmcr_ro | PMU_PMCR_E | pmcr_lp);
 	isb();
 
 	for (i = 0; i < 100; i++)
@@ -886,12 +901,12 @@ static void test_overflow_interrupt(bool overflow_at_64bits)
 
 	pmu_reset_stats();
 
-	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW);
-	write_regn_el0(pmevcntr, 1, PRE_OVERFLOW);
+	write_regn_el0(pmevcntr, 0, pre_overflow);
+	write_regn_el0(pmevcntr, 1, pre_overflow);
 	write_sysreg(ALL_SET, pmintenset_el1);
 	isb();
 
-	mem_access_loop(addr, 200, pmu.pmcr_ro | PMU_PMCR_E);
+	mem_access_loop(addr, 200, pmu.pmcr_ro | PMU_PMCR_E | pmcr_lp);
 	for (i = 0; i < 100; i++)
 		write_sysreg(0x3, pmswinc_el0);
 
@@ -900,25 +915,35 @@ static void test_overflow_interrupt(bool overflow_at_64bits)
 	report(expect_interrupts(0x3),
 		"overflow interrupts expected on #0 and #1");
 
-	/* promote to 64-b */
+	/*
+	 * promote to 64-b:
+	 *
+	 * This only applies to the !overflow_at_64bits case, as
+	 * overflow_at_64bits doesn't implement CHAIN events. The
+	 * overflow_at_64bits case just checks that chained counters are
+	 * not incremented when PMCR.LP == 1.
+	 */
 
 	pmu_reset_stats();
 
 	write_regn_el0(pmevtyper, 1, CHAIN | PMEVTYPER_EXCLUDE_EL0);
-	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW);
+	write_regn_el0(pmevcntr, 0, pre_overflow);
 	isb();
-	mem_access_loop(addr, 200, pmu.pmcr_ro | PMU_PMCR_E);
-	report(expect_interrupts(0x1),
-		"expect overflow interrupt on 32b boundary");
+	mem_access_loop(addr, 200, pmu.pmcr_ro | PMU_PMCR_E | pmcr_lp);
+	report(expect_interrupts(0x1), "expect overflow interrupt");
 
 	/* overflow on odd counter */
 	pmu_reset_stats();
-	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW);
-	write_regn_el0(pmevcntr, 1, ALL_SET);
+	write_regn_el0(pmevcntr, 0, pre_overflow);
+	write_regn_el0(pmevcntr, 1, all_set);
 	isb();
-	mem_access_loop(addr, 400, pmu.pmcr_ro | PMU_PMCR_E);
-	report(expect_interrupts(0x3),
-		"expect overflow interrupt on even and odd counter");
+	mem_access_loop(addr, 400, pmu.pmcr_ro | PMU_PMCR_E | pmcr_lp);
+	if (overflow_at_64bits)
+		report(expect_interrupts(0x1),
+		       "expect overflow interrupt on even counter");
+	else
+		report(expect_interrupts(0x3),
+		       "expect overflow interrupt on even and odd counter");
 }
 #endif
 
@@ -1119,10 +1144,13 @@ int main(int argc, char *argv[])
 		report_prefix_pop();
 	} else if (strcmp(argv[1], "pmu-basic-event-count") == 0) {
 		run_test(argv[1], test_basic_event_count, false);
+		run_test(argv[1], test_basic_event_count, true);
 	} else if (strcmp(argv[1], "pmu-mem-access") == 0) {
 		run_test(argv[1], test_mem_access, false);
+		run_test(argv[1], test_mem_access, true);
 	} else if (strcmp(argv[1], "pmu-sw-incr") == 0) {
 		run_test(argv[1], test_sw_incr, false);
+		run_test(argv[1], test_sw_incr, true);
 	} else if (strcmp(argv[1], "pmu-chained-counters") == 0) {
 		run_test(argv[1], test_chained_counters, false);
 	} else if (strcmp(argv[1], "pmu-chained-sw-incr") == 0) {
@@ -1131,6 +1159,7 @@ int main(int argc, char *argv[])
 		run_test(argv[1], test_chain_promotion, false);
 	} else if (strcmp(argv[1], "pmu-overflow-interrupt") == 0) {
 		run_test(argv[1], test_overflow_interrupt, false);
+		run_test(argv[1], test_overflow_interrupt, true);
 	} else {
 		report_abort("Unknown sub-test '%s'", argv[1]);
 	}
-- 
2.39.0.rc0.267.gcb52ba06e7-goog

Re: [kvm-unit-tests PATCH 3/3] arm: pmu: Add tests for 64-bit overflows

[Alexandru Elisei]

Hi,

Checked that all places where ALL_SET/PRE_OVERFLOW were used are now taking
into account the fact that counters are programmed to be 64bit.

In the case of 64bit counters, the printf format specifier is %ld, which
means that ALL_SET_64 and PRE_OVERFLOW_64 are now displayed as negative
numbers. For example:

INFO: pmu: pmu-sw-incr: 32-bit: SW_INCR counter #0 has value 4294967280
PASS: pmu: pmu-sw-incr: 32-bit: PWSYNC does not increment if PMCR.E is unset
[..]
INFO: pmu: pmu-sw-incr: 64-bit: SW_INCR counter #0 has value -16
PASS: pmu: pmu-sw-incr: 64-bit: PWSYNC does not increment if PMCR.E is unset

I was thinking that the format specifiers could be changed to unsigned
long.  The counters only increment, they don't decrement, and I can't think
how printing them as signed could be useful.

One more comment below.

On Fri, Dec 02, 2022 at 04:55:27AM +0000, Ricardo Koller wrote:
> Modify all tests checking overflows to support both 32 (PMCR_EL0.LP == 0)
> and 64-bit overflows (PMCR_EL0.LP == 1). 64-bit overflows are only
> supported on PMUv3p5.
> 
> Note that chained tests do not implement "overflow_at_64bits == true".
> That's because there are no CHAIN events when "PMCR_EL0.LP == 1" (for more
> details see AArch64.IncrementEventCounter() pseudocode in the ARM ARM DDI
> 0487H.a, J1.1.1 "aarch64/debug").
> 
> Signed-off-by: Ricardo Koller <ricarkol@google.com>
> ---
>  arm/pmu.c | 91 ++++++++++++++++++++++++++++++++++++-------------------
>  1 file changed, 60 insertions(+), 31 deletions(-)
> 
> diff --git a/arm/pmu.c b/arm/pmu.c
> index 59e5bfe..3cb563b 100644
> --- a/arm/pmu.c
> +++ b/arm/pmu.c
> @@ -28,6 +28,7 @@
>  #define PMU_PMCR_X         (1 << 4)
>  #define PMU_PMCR_DP        (1 << 5)
>  #define PMU_PMCR_LC        (1 << 6)
> +#define PMU_PMCR_LP        (1 << 7)
>  #define PMU_PMCR_N_SHIFT   11
>  #define PMU_PMCR_N_MASK    0x1f
>  #define PMU_PMCR_ID_SHIFT  16
> @@ -55,10 +56,15 @@
>  #define EXT_COMMON_EVENTS_HIGH	0x403F
>  
>  #define ALL_SET			0x00000000FFFFFFFFULL
> +#define ALL_SET_64		0xFFFFFFFFFFFFFFFFULL
>  #define ALL_CLEAR		0x0000000000000000ULL
>  #define PRE_OVERFLOW		0x00000000FFFFFFF0ULL
> +#define PRE_OVERFLOW_64		0xFFFFFFFFFFFFFFF0ULL
>  #define PRE_OVERFLOW2		0x00000000FFFFFFDCULL
>  
> +#define PRE_OVERFLOW_AT(_64b)	(_64b ? PRE_OVERFLOW_64 : PRE_OVERFLOW)
> +#define ALL_SET_AT(_64b)	(_64b ? ALL_SET_64 : ALL_SET)
> +
>  #define PMU_PPI			23
>  
>  struct pmu {
> @@ -429,8 +435,10 @@ static bool satisfy_prerequisites(uint32_t *events, unsigned int nb_events,
>  static void test_basic_event_count(bool overflow_at_64bits)
>  {
>  	uint32_t implemented_counter_mask, non_implemented_counter_mask;
> -	uint32_t counter_mask;
> +	uint64_t pre_overflow = PRE_OVERFLOW_AT(overflow_at_64bits);
> +	uint64_t pmcr_lp = overflow_at_64bits ? PMU_PMCR_LP : 0;
>  	uint32_t events[] = {CPU_CYCLES, INST_RETIRED};
> +	uint32_t counter_mask;
>  
>  	if (!satisfy_prerequisites(events, ARRAY_SIZE(events),
>  				   overflow_at_64bits))
> @@ -452,13 +460,13 @@ static void test_basic_event_count(bool overflow_at_64bits)
>  	 * 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);
> +	set_pmcr(pmu.pmcr_ro | PMU_PMCR_LC | PMU_PMCR_C | PMU_PMCR_P | pmcr_lp);
>  	isb();
> -	report(get_pmcr() == (pmu.pmcr_ro | PMU_PMCR_LC), "pmcr: reset counters");
> +	report(get_pmcr() == (pmu.pmcr_ro | PMU_PMCR_LC | pmcr_lp), "pmcr: reset counters");
>  
>  	/* Preset counter #0 to pre overflow value to trigger an overflow */
> -	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW);
> -	report(read_regn_el0(pmevcntr, 0) == PRE_OVERFLOW,
> +	write_regn_el0(pmevcntr, 0, pre_overflow);
> +	report(read_regn_el0(pmevcntr, 0) == pre_overflow,
>  		"counter #0 preset to pre-overflow value");
>  	report(!read_regn_el0(pmevcntr, 1), "counter #1 is 0");
>  
> @@ -511,6 +519,8 @@ static void test_mem_access(bool overflow_at_64bits)
>  {
>  	void *addr = malloc(PAGE_SIZE);
>  	uint32_t events[] = {MEM_ACCESS, MEM_ACCESS};
> +	uint64_t pre_overflow = PRE_OVERFLOW_AT(overflow_at_64bits);
> +	uint64_t pmcr_lp = overflow_at_64bits ? PMU_PMCR_LP : 0;
>  
>  	if (!satisfy_prerequisites(events, ARRAY_SIZE(events),
>  				   overflow_at_64bits))
> @@ -522,7 +532,7 @@ static void test_mem_access(bool overflow_at_64bits)
>  	write_regn_el0(pmevtyper, 1, MEM_ACCESS | PMEVTYPER_EXCLUDE_EL0);
>  	write_sysreg_s(0x3, PMCNTENSET_EL0);
>  	isb();
> -	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E);
> +	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E | pmcr_lp);
>  	report_info("counter #0 is %ld (MEM_ACCESS)", read_regn_el0(pmevcntr, 0));
>  	report_info("counter #1 is %ld (MEM_ACCESS)", read_regn_el0(pmevcntr, 1));
>  	/* We may measure more than 20 mem access depending on the core */
> @@ -532,11 +542,11 @@ static void test_mem_access(bool overflow_at_64bits)
>  
>  	pmu_reset();
>  
> -	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW);
> -	write_regn_el0(pmevcntr, 1, PRE_OVERFLOW);
> +	write_regn_el0(pmevcntr, 0, pre_overflow);
> +	write_regn_el0(pmevcntr, 1, pre_overflow);
>  	write_sysreg_s(0x3, PMCNTENSET_EL0);
>  	isb();
> -	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E);
> +	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E | pmcr_lp);
>  	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",
> @@ -546,6 +556,8 @@ static void test_mem_access(bool overflow_at_64bits)
>  
>  static void test_sw_incr(bool overflow_at_64bits)
>  {
> +	uint64_t pre_overflow = PRE_OVERFLOW_AT(overflow_at_64bits);
> +	uint64_t pmcr_lp = overflow_at_64bits ? PMU_PMCR_LP : 0;
>  	uint32_t events[] = {SW_INCR, SW_INCR};
>  	uint64_t cntr0;
>  	int i;
> @@ -561,7 +573,7 @@ static void test_sw_incr(bool overflow_at_64bits)
>  	/* enable counters #0 and #1 */
>  	write_sysreg_s(0x3, PMCNTENSET_EL0);
>  
> -	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW);
> +	write_regn_el0(pmevcntr, 0, pre_overflow);
>  	isb();
>  
>  	for (i = 0; i < 100; i++)
> @@ -569,21 +581,21 @@ static void test_sw_incr(bool overflow_at_64bits)
>  
>  	isb();
>  	report_info("SW_INCR counter #0 has value %ld", read_regn_el0(pmevcntr, 0));
> -	report(read_regn_el0(pmevcntr, 0) == PRE_OVERFLOW,
> +	report(read_regn_el0(pmevcntr, 0) == pre_overflow,
>  		"PWSYNC does not increment if PMCR.E is unset");
>  
>  	pmu_reset();
>  
> -	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW);
> +	write_regn_el0(pmevcntr, 0, pre_overflow);
>  	write_sysreg_s(0x3, PMCNTENSET_EL0);
> -	set_pmcr(pmu.pmcr_ro | PMU_PMCR_E);
> +	set_pmcr(pmu.pmcr_ro | PMU_PMCR_E | pmcr_lp);
>  	isb();
>  
>  	for (i = 0; i < 100; i++)
>  		write_sysreg(0x3, pmswinc_el0);
>  
>  	isb();
> -	cntr0 = (pmu.version < ID_DFR0_PMU_V3_8_5) ? 84 : PRE_OVERFLOW + 100;
> +	cntr0 = (pmu.version < ID_DFR0_PMU_V3_8_5) ? 84 : pre_overflow + 100;
>  	report(read_regn_el0(pmevcntr, 0) == cntr0, "counter #0 after + 100 SW_INCR");
>  	report(read_regn_el0(pmevcntr, 1) == 100, "counter #1 after + 100 SW_INCR");
>  	report_info("counter values after 100 SW_INCR #0=%ld #1=%ld",
> @@ -844,6 +856,9 @@ static bool expect_interrupts(uint32_t bitmap)
>  
>  static void test_overflow_interrupt(bool overflow_at_64bits)
>  {
> +	uint64_t pre_overflow = PRE_OVERFLOW_AT(overflow_at_64bits);
> +	uint64_t all_set = ALL_SET_AT(overflow_at_64bits);
> +	uint64_t pmcr_lp = overflow_at_64bits ? PMU_PMCR_LP : 0;
>  	uint32_t events[] = {MEM_ACCESS, SW_INCR};
>  	void *addr = malloc(PAGE_SIZE);
>  	int i;
> @@ -862,16 +877,16 @@ static void test_overflow_interrupt(bool overflow_at_64bits)
>  	write_regn_el0(pmevtyper, 0, MEM_ACCESS | PMEVTYPER_EXCLUDE_EL0);
>  	write_regn_el0(pmevtyper, 1, SW_INCR | PMEVTYPER_EXCLUDE_EL0);
>  	write_sysreg_s(0x3, PMCNTENSET_EL0);
> -	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW);
> -	write_regn_el0(pmevcntr, 1, PRE_OVERFLOW);
> +	write_regn_el0(pmevcntr, 0, pre_overflow);
> +	write_regn_el0(pmevcntr, 1, pre_overflow);
>  	isb();
>  
>  	/* interrupts are disabled (PMINTENSET_EL1 == 0) */
>  
> -	mem_access_loop(addr, 200, pmu.pmcr_ro | PMU_PMCR_E);
> +	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E | pmcr_lp);
>  	report(expect_interrupts(0), "no overflow interrupt after preset");
>  
> -	set_pmcr(pmu.pmcr_ro | PMU_PMCR_E);
> +	set_pmcr(pmu.pmcr_ro | PMU_PMCR_E | pmcr_lp);
>  	isb();
>  
>  	for (i = 0; i < 100; i++)
> @@ -886,12 +901,12 @@ static void test_overflow_interrupt(bool overflow_at_64bits)
>  
>  	pmu_reset_stats();
>  
> -	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW);
> -	write_regn_el0(pmevcntr, 1, PRE_OVERFLOW);
> +	write_regn_el0(pmevcntr, 0, pre_overflow);
> +	write_regn_el0(pmevcntr, 1, pre_overflow);
>  	write_sysreg(ALL_SET, pmintenset_el1);
>  	isb();
>  
> -	mem_access_loop(addr, 200, pmu.pmcr_ro | PMU_PMCR_E);
> +	mem_access_loop(addr, 200, pmu.pmcr_ro | PMU_PMCR_E | pmcr_lp);
>  	for (i = 0; i < 100; i++)
>  		write_sysreg(0x3, pmswinc_el0);
>  
> @@ -900,25 +915,35 @@ static void test_overflow_interrupt(bool overflow_at_64bits)
>  	report(expect_interrupts(0x3),
>  		"overflow interrupts expected on #0 and #1");
>  
> -	/* promote to 64-b */
> +	/*
> +	 * promote to 64-b:
> +	 *
> +	 * This only applies to the !overflow_at_64bits case, as
> +	 * overflow_at_64bits doesn't implement CHAIN events. The
> +	 * overflow_at_64bits case just checks that chained counters are
> +	 * not incremented when PMCR.LP == 1.
> +	 */

If this doesn't do anything for when overflow_at_64bits, and since the
interrupt is already tested before this part, why not exit early?

Or the test could check that the CHAIN event indeed does not increment when
LP=1 at the end of this function.

Thanks,
Alex

>  
>  	pmu_reset_stats();
>  
>  	write_regn_el0(pmevtyper, 1, CHAIN | PMEVTYPER_EXCLUDE_EL0);
> -	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW);
> +	write_regn_el0(pmevcntr, 0, pre_overflow);
>  	isb();
> -	mem_access_loop(addr, 200, pmu.pmcr_ro | PMU_PMCR_E);
> -	report(expect_interrupts(0x1),
> -		"expect overflow interrupt on 32b boundary");
> +	mem_access_loop(addr, 200, pmu.pmcr_ro | PMU_PMCR_E | pmcr_lp);
> +	report(expect_interrupts(0x1), "expect overflow interrupt");
>  
>  	/* overflow on odd counter */
>  	pmu_reset_stats();
> -	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW);
> -	write_regn_el0(pmevcntr, 1, ALL_SET);
> +	write_regn_el0(pmevcntr, 0, pre_overflow);
> +	write_regn_el0(pmevcntr, 1, all_set);
>  	isb();
> -	mem_access_loop(addr, 400, pmu.pmcr_ro | PMU_PMCR_E);
> -	report(expect_interrupts(0x3),
> -		"expect overflow interrupt on even and odd counter");
> +	mem_access_loop(addr, 400, pmu.pmcr_ro | PMU_PMCR_E | pmcr_lp);
> +	if (overflow_at_64bits)
> +		report(expect_interrupts(0x1),
> +		       "expect overflow interrupt on even counter");
> +	else
> +		report(expect_interrupts(0x3),
> +		       "expect overflow interrupt on even and odd counter");
>  }
>  #endif
>  
> @@ -1119,10 +1144,13 @@ int main(int argc, char *argv[])
>  		report_prefix_pop();
>  	} else if (strcmp(argv[1], "pmu-basic-event-count") == 0) {
>  		run_test(argv[1], test_basic_event_count, false);
> +		run_test(argv[1], test_basic_event_count, true);
>  	} else if (strcmp(argv[1], "pmu-mem-access") == 0) {
>  		run_test(argv[1], test_mem_access, false);
> +		run_test(argv[1], test_mem_access, true);
>  	} else if (strcmp(argv[1], "pmu-sw-incr") == 0) {
>  		run_test(argv[1], test_sw_incr, false);
> +		run_test(argv[1], test_sw_incr, true);
>  	} else if (strcmp(argv[1], "pmu-chained-counters") == 0) {
>  		run_test(argv[1], test_chained_counters, false);
>  	} else if (strcmp(argv[1], "pmu-chained-sw-incr") == 0) {
> @@ -1131,6 +1159,7 @@ int main(int argc, char *argv[])
>  		run_test(argv[1], test_chain_promotion, false);
>  	} else if (strcmp(argv[1], "pmu-overflow-interrupt") == 0) {
>  		run_test(argv[1], test_overflow_interrupt, false);
> +		run_test(argv[1], test_overflow_interrupt, true);
>  	} else {
>  		report_abort("Unknown sub-test '%s'", argv[1]);
>  	}
> -- 
> 2.39.0.rc0.267.gcb52ba06e7-goog
> 

Re: [kvm-unit-tests PATCH 3/3] arm: pmu: Add tests for 64-bit overflows

[Ricardo Koller]

On Tue, Dec 13, 2022 at 05:03:40PM +0000, Alexandru Elisei wrote:
> Hi,
> 
> Checked that all places where ALL_SET/PRE_OVERFLOW were used are now taking
> into account the fact that counters are programmed to be 64bit.
> 
> In the case of 64bit counters, the printf format specifier is %ld, which
> means that ALL_SET_64 and PRE_OVERFLOW_64 are now displayed as negative
> numbers. For example:
> 
> INFO: pmu: pmu-sw-incr: 32-bit: SW_INCR counter #0 has value 4294967280
> PASS: pmu: pmu-sw-incr: 32-bit: PWSYNC does not increment if PMCR.E is unset
> [..]
> INFO: pmu: pmu-sw-incr: 64-bit: SW_INCR counter #0 has value -16
> PASS: pmu: pmu-sw-incr: 64-bit: PWSYNC does not increment if PMCR.E is unset
> 

Argh, I didn't notice this. I think this would be more useful if it
printed %llx in all cases.

> I was thinking that the format specifiers could be changed to unsigned
> long.  The counters only increment, they don't decrement, and I can't think
> how printing them as signed could be useful.
> 
> One more comment below.
> 
> On Fri, Dec 02, 2022 at 04:55:27AM +0000, Ricardo Koller wrote:
> > Modify all tests checking overflows to support both 32 (PMCR_EL0.LP == 0)
> > and 64-bit overflows (PMCR_EL0.LP == 1). 64-bit overflows are only
> > supported on PMUv3p5.
> > 
> > Note that chained tests do not implement "overflow_at_64bits == true".
> > That's because there are no CHAIN events when "PMCR_EL0.LP == 1" (for more
> > details see AArch64.IncrementEventCounter() pseudocode in the ARM ARM DDI
> > 0487H.a, J1.1.1 "aarch64/debug").
> > 
> > Signed-off-by: Ricardo Koller <ricarkol@google.com>
> > ---
> >  arm/pmu.c | 91 ++++++++++++++++++++++++++++++++++++-------------------
> >  1 file changed, 60 insertions(+), 31 deletions(-)
> > 
> > diff --git a/arm/pmu.c b/arm/pmu.c
> > index 59e5bfe..3cb563b 100644
> > --- a/arm/pmu.c
> > +++ b/arm/pmu.c
> > @@ -28,6 +28,7 @@
> >  #define PMU_PMCR_X         (1 << 4)
> >  #define PMU_PMCR_DP        (1 << 5)
> >  #define PMU_PMCR_LC        (1 << 6)
> > +#define PMU_PMCR_LP        (1 << 7)
> >  #define PMU_PMCR_N_SHIFT   11
> >  #define PMU_PMCR_N_MASK    0x1f
> >  #define PMU_PMCR_ID_SHIFT  16
> > @@ -55,10 +56,15 @@
> >  #define EXT_COMMON_EVENTS_HIGH	0x403F
> >  
> >  #define ALL_SET			0x00000000FFFFFFFFULL
> > +#define ALL_SET_64		0xFFFFFFFFFFFFFFFFULL
> >  #define ALL_CLEAR		0x0000000000000000ULL
> >  #define PRE_OVERFLOW		0x00000000FFFFFFF0ULL
> > +#define PRE_OVERFLOW_64		0xFFFFFFFFFFFFFFF0ULL
> >  #define PRE_OVERFLOW2		0x00000000FFFFFFDCULL
> >  
> > +#define PRE_OVERFLOW_AT(_64b)	(_64b ? PRE_OVERFLOW_64 : PRE_OVERFLOW)
> > +#define ALL_SET_AT(_64b)	(_64b ? ALL_SET_64 : ALL_SET)
> > +
> >  #define PMU_PPI			23
> >  
> >  struct pmu {
> > @@ -429,8 +435,10 @@ static bool satisfy_prerequisites(uint32_t *events, unsigned int nb_events,
> >  static void test_basic_event_count(bool overflow_at_64bits)
> >  {
> >  	uint32_t implemented_counter_mask, non_implemented_counter_mask;
> > -	uint32_t counter_mask;
> > +	uint64_t pre_overflow = PRE_OVERFLOW_AT(overflow_at_64bits);
> > +	uint64_t pmcr_lp = overflow_at_64bits ? PMU_PMCR_LP : 0;
> >  	uint32_t events[] = {CPU_CYCLES, INST_RETIRED};
> > +	uint32_t counter_mask;
> >  
> >  	if (!satisfy_prerequisites(events, ARRAY_SIZE(events),
> >  				   overflow_at_64bits))
> > @@ -452,13 +460,13 @@ static void test_basic_event_count(bool overflow_at_64bits)
> >  	 * 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);
> > +	set_pmcr(pmu.pmcr_ro | PMU_PMCR_LC | PMU_PMCR_C | PMU_PMCR_P | pmcr_lp);
> >  	isb();
> > -	report(get_pmcr() == (pmu.pmcr_ro | PMU_PMCR_LC), "pmcr: reset counters");
> > +	report(get_pmcr() == (pmu.pmcr_ro | PMU_PMCR_LC | pmcr_lp), "pmcr: reset counters");
> >  
> >  	/* Preset counter #0 to pre overflow value to trigger an overflow */
> > -	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW);
> > -	report(read_regn_el0(pmevcntr, 0) == PRE_OVERFLOW,
> > +	write_regn_el0(pmevcntr, 0, pre_overflow);
> > +	report(read_regn_el0(pmevcntr, 0) == pre_overflow,
> >  		"counter #0 preset to pre-overflow value");
> >  	report(!read_regn_el0(pmevcntr, 1), "counter #1 is 0");
> >  
> > @@ -511,6 +519,8 @@ static void test_mem_access(bool overflow_at_64bits)
> >  {
> >  	void *addr = malloc(PAGE_SIZE);
> >  	uint32_t events[] = {MEM_ACCESS, MEM_ACCESS};
> > +	uint64_t pre_overflow = PRE_OVERFLOW_AT(overflow_at_64bits);
> > +	uint64_t pmcr_lp = overflow_at_64bits ? PMU_PMCR_LP : 0;
> >  
> >  	if (!satisfy_prerequisites(events, ARRAY_SIZE(events),
> >  				   overflow_at_64bits))
> > @@ -522,7 +532,7 @@ static void test_mem_access(bool overflow_at_64bits)
> >  	write_regn_el0(pmevtyper, 1, MEM_ACCESS | PMEVTYPER_EXCLUDE_EL0);
> >  	write_sysreg_s(0x3, PMCNTENSET_EL0);
> >  	isb();
> > -	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E);
> > +	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E | pmcr_lp);
> >  	report_info("counter #0 is %ld (MEM_ACCESS)", read_regn_el0(pmevcntr, 0));
> >  	report_info("counter #1 is %ld (MEM_ACCESS)", read_regn_el0(pmevcntr, 1));
> >  	/* We may measure more than 20 mem access depending on the core */
> > @@ -532,11 +542,11 @@ static void test_mem_access(bool overflow_at_64bits)
> >  
> >  	pmu_reset();
> >  
> > -	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW);
> > -	write_regn_el0(pmevcntr, 1, PRE_OVERFLOW);
> > +	write_regn_el0(pmevcntr, 0, pre_overflow);
> > +	write_regn_el0(pmevcntr, 1, pre_overflow);
> >  	write_sysreg_s(0x3, PMCNTENSET_EL0);
> >  	isb();
> > -	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E);
> > +	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E | pmcr_lp);
> >  	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",
> > @@ -546,6 +556,8 @@ static void test_mem_access(bool overflow_at_64bits)
> >  
> >  static void test_sw_incr(bool overflow_at_64bits)
> >  {
> > +	uint64_t pre_overflow = PRE_OVERFLOW_AT(overflow_at_64bits);
> > +	uint64_t pmcr_lp = overflow_at_64bits ? PMU_PMCR_LP : 0;
> >  	uint32_t events[] = {SW_INCR, SW_INCR};
> >  	uint64_t cntr0;
> >  	int i;
> > @@ -561,7 +573,7 @@ static void test_sw_incr(bool overflow_at_64bits)
> >  	/* enable counters #0 and #1 */
> >  	write_sysreg_s(0x3, PMCNTENSET_EL0);
> >  
> > -	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW);
> > +	write_regn_el0(pmevcntr, 0, pre_overflow);
> >  	isb();
> >  
> >  	for (i = 0; i < 100; i++)
> > @@ -569,21 +581,21 @@ static void test_sw_incr(bool overflow_at_64bits)
> >  
> >  	isb();
> >  	report_info("SW_INCR counter #0 has value %ld", read_regn_el0(pmevcntr, 0));
> > -	report(read_regn_el0(pmevcntr, 0) == PRE_OVERFLOW,
> > +	report(read_regn_el0(pmevcntr, 0) == pre_overflow,
> >  		"PWSYNC does not increment if PMCR.E is unset");
> >  
> >  	pmu_reset();
> >  
> > -	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW);
> > +	write_regn_el0(pmevcntr, 0, pre_overflow);
> >  	write_sysreg_s(0x3, PMCNTENSET_EL0);
> > -	set_pmcr(pmu.pmcr_ro | PMU_PMCR_E);
> > +	set_pmcr(pmu.pmcr_ro | PMU_PMCR_E | pmcr_lp);
> >  	isb();
> >  
> >  	for (i = 0; i < 100; i++)
> >  		write_sysreg(0x3, pmswinc_el0);
> >  
> >  	isb();
> > -	cntr0 = (pmu.version < ID_DFR0_PMU_V3_8_5) ? 84 : PRE_OVERFLOW + 100;
> > +	cntr0 = (pmu.version < ID_DFR0_PMU_V3_8_5) ? 84 : pre_overflow + 100;
> >  	report(read_regn_el0(pmevcntr, 0) == cntr0, "counter #0 after + 100 SW_INCR");
> >  	report(read_regn_el0(pmevcntr, 1) == 100, "counter #1 after + 100 SW_INCR");
> >  	report_info("counter values after 100 SW_INCR #0=%ld #1=%ld",
> > @@ -844,6 +856,9 @@ static bool expect_interrupts(uint32_t bitmap)
> >  
> >  static void test_overflow_interrupt(bool overflow_at_64bits)
> >  {
> > +	uint64_t pre_overflow = PRE_OVERFLOW_AT(overflow_at_64bits);
> > +	uint64_t all_set = ALL_SET_AT(overflow_at_64bits);
> > +	uint64_t pmcr_lp = overflow_at_64bits ? PMU_PMCR_LP : 0;
> >  	uint32_t events[] = {MEM_ACCESS, SW_INCR};
> >  	void *addr = malloc(PAGE_SIZE);
> >  	int i;
> > @@ -862,16 +877,16 @@ static void test_overflow_interrupt(bool overflow_at_64bits)
> >  	write_regn_el0(pmevtyper, 0, MEM_ACCESS | PMEVTYPER_EXCLUDE_EL0);
> >  	write_regn_el0(pmevtyper, 1, SW_INCR | PMEVTYPER_EXCLUDE_EL0);
> >  	write_sysreg_s(0x3, PMCNTENSET_EL0);
> > -	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW);
> > -	write_regn_el0(pmevcntr, 1, PRE_OVERFLOW);
> > +	write_regn_el0(pmevcntr, 0, pre_overflow);
> > +	write_regn_el0(pmevcntr, 1, pre_overflow);
> >  	isb();
> >  
> >  	/* interrupts are disabled (PMINTENSET_EL1 == 0) */
> >  
> > -	mem_access_loop(addr, 200, pmu.pmcr_ro | PMU_PMCR_E);
> > +	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E | pmcr_lp);
> >  	report(expect_interrupts(0), "no overflow interrupt after preset");
> >  
> > -	set_pmcr(pmu.pmcr_ro | PMU_PMCR_E);
> > +	set_pmcr(pmu.pmcr_ro | PMU_PMCR_E | pmcr_lp);
> >  	isb();
> >  
> >  	for (i = 0; i < 100; i++)
> > @@ -886,12 +901,12 @@ static void test_overflow_interrupt(bool overflow_at_64bits)
> >  
> >  	pmu_reset_stats();
> >  
> > -	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW);
> > -	write_regn_el0(pmevcntr, 1, PRE_OVERFLOW);
> > +	write_regn_el0(pmevcntr, 0, pre_overflow);
> > +	write_regn_el0(pmevcntr, 1, pre_overflow);
> >  	write_sysreg(ALL_SET, pmintenset_el1);
> >  	isb();
> >  
> > -	mem_access_loop(addr, 200, pmu.pmcr_ro | PMU_PMCR_E);
> > +	mem_access_loop(addr, 200, pmu.pmcr_ro | PMU_PMCR_E | pmcr_lp);
> >  	for (i = 0; i < 100; i++)
> >  		write_sysreg(0x3, pmswinc_el0);
> >  
> > @@ -900,25 +915,35 @@ static void test_overflow_interrupt(bool overflow_at_64bits)
> >  	report(expect_interrupts(0x3),
> >  		"overflow interrupts expected on #0 and #1");
> >  
> > -	/* promote to 64-b */
> > +	/*
> > +	 * promote to 64-b:
> > +	 *
> > +	 * This only applies to the !overflow_at_64bits case, as
> > +	 * overflow_at_64bits doesn't implement CHAIN events. The
> > +	 * overflow_at_64bits case just checks that chained counters are
> > +	 * not incremented when PMCR.LP == 1.
> > +	 */
> 
> If this doesn't do anything for when overflow_at_64bits, and since the
> interrupt is already tested before this part, why not exit early?
> 
> Or the test could check that the CHAIN event indeed does not increment when
> LP=1 at the end of this function.

That's precisely what it's doing.

Thanks!
Ricardo

> 
> Thanks,
> Alex
> 
> >  
> >  	pmu_reset_stats();
> >  
> >  	write_regn_el0(pmevtyper, 1, CHAIN | PMEVTYPER_EXCLUDE_EL0);
> > -	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW);
> > +	write_regn_el0(pmevcntr, 0, pre_overflow);
> >  	isb();
> > -	mem_access_loop(addr, 200, pmu.pmcr_ro | PMU_PMCR_E);
> > -	report(expect_interrupts(0x1),
> > -		"expect overflow interrupt on 32b boundary");
> > +	mem_access_loop(addr, 200, pmu.pmcr_ro | PMU_PMCR_E | pmcr_lp);
> > +	report(expect_interrupts(0x1), "expect overflow interrupt");
> >  
> >  	/* overflow on odd counter */
> >  	pmu_reset_stats();
> > -	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW);
> > -	write_regn_el0(pmevcntr, 1, ALL_SET);
> > +	write_regn_el0(pmevcntr, 0, pre_overflow);
> > +	write_regn_el0(pmevcntr, 1, all_set);
> >  	isb();
> > -	mem_access_loop(addr, 400, pmu.pmcr_ro | PMU_PMCR_E);
> > -	report(expect_interrupts(0x3),
> > -		"expect overflow interrupt on even and odd counter");
> > +	mem_access_loop(addr, 400, pmu.pmcr_ro | PMU_PMCR_E | pmcr_lp);
> > +	if (overflow_at_64bits)
> > +		report(expect_interrupts(0x1),
> > +		       "expect overflow interrupt on even counter");
> > +	else
> > +		report(expect_interrupts(0x3),
> > +		       "expect overflow interrupt on even and odd counter");
> >  }
> >  #endif
> >  
> > @@ -1119,10 +1144,13 @@ int main(int argc, char *argv[])
> >  		report_prefix_pop();
> >  	} else if (strcmp(argv[1], "pmu-basic-event-count") == 0) {
> >  		run_test(argv[1], test_basic_event_count, false);
> > +		run_test(argv[1], test_basic_event_count, true);
> >  	} else if (strcmp(argv[1], "pmu-mem-access") == 0) {
> >  		run_test(argv[1], test_mem_access, false);
> > +		run_test(argv[1], test_mem_access, true);
> >  	} else if (strcmp(argv[1], "pmu-sw-incr") == 0) {
> >  		run_test(argv[1], test_sw_incr, false);
> > +		run_test(argv[1], test_sw_incr, true);
> >  	} else if (strcmp(argv[1], "pmu-chained-counters") == 0) {
> >  		run_test(argv[1], test_chained_counters, false);
> >  	} else if (strcmp(argv[1], "pmu-chained-sw-incr") == 0) {
> > @@ -1131,6 +1159,7 @@ int main(int argc, char *argv[])
> >  		run_test(argv[1], test_chain_promotion, false);
> >  	} else if (strcmp(argv[1], "pmu-overflow-interrupt") == 0) {
> >  		run_test(argv[1], test_overflow_interrupt, false);
> > +		run_test(argv[1], test_overflow_interrupt, true);
> >  	} else {
> >  		report_abort("Unknown sub-test '%s'", argv[1]);
> >  	}
> > -- 
> > 2.39.0.rc0.267.gcb52ba06e7-goog
> > 

Re: [kvm-unit-tests PATCH 3/3] arm: pmu: Add tests for 64-bit overflows

[Alexandru Elisei]

Hi,

On Tue, Dec 13, 2022 at 10:04:04AM -0800, Ricardo Koller wrote:
> On Tue, Dec 13, 2022 at 05:03:40PM +0000, Alexandru Elisei wrote:
> > Hi,
> > 
> > Checked that all places where ALL_SET/PRE_OVERFLOW were used are now taking
> > into account the fact that counters are programmed to be 64bit.
> > 
> > In the case of 64bit counters, the printf format specifier is %ld, which
> > means that ALL_SET_64 and PRE_OVERFLOW_64 are now displayed as negative
> > numbers. For example:
> > 
> > INFO: pmu: pmu-sw-incr: 32-bit: SW_INCR counter #0 has value 4294967280
> > PASS: pmu: pmu-sw-incr: 32-bit: PWSYNC does not increment if PMCR.E is unset
> > [..]
> > INFO: pmu: pmu-sw-incr: 64-bit: SW_INCR counter #0 has value -16
> > PASS: pmu: pmu-sw-incr: 64-bit: PWSYNC does not increment if PMCR.E is unset
> > 
> 
> Argh, I didn't notice this. I think this would be more useful if it
> printed %llx in all cases.

Indeed, printing the hex value will make it easier to match it against the
defines.

One more comment below.

> 
> > I was thinking that the format specifiers could be changed to unsigned
> > long.  The counters only increment, they don't decrement, and I can't think
> > how printing them as signed could be useful.
> > 
> > One more comment below.
> > 
> > On Fri, Dec 02, 2022 at 04:55:27AM +0000, Ricardo Koller wrote:
> > > Modify all tests checking overflows to support both 32 (PMCR_EL0.LP == 0)
> > > and 64-bit overflows (PMCR_EL0.LP == 1). 64-bit overflows are only
> > > supported on PMUv3p5.
> > > 
> > > Note that chained tests do not implement "overflow_at_64bits == true".
> > > That's because there are no CHAIN events when "PMCR_EL0.LP == 1" (for more
> > > details see AArch64.IncrementEventCounter() pseudocode in the ARM ARM DDI
> > > 0487H.a, J1.1.1 "aarch64/debug").
> > > 
> > > Signed-off-by: Ricardo Koller <ricarkol@google.com>
> > > ---
> > >  arm/pmu.c | 91 ++++++++++++++++++++++++++++++++++++-------------------
> > >  1 file changed, 60 insertions(+), 31 deletions(-)
> > > 
> > > diff --git a/arm/pmu.c b/arm/pmu.c
> > > index 59e5bfe..3cb563b 100644
> > > --- a/arm/pmu.c
> > > +++ b/arm/pmu.c
> > > @@ -28,6 +28,7 @@
> > >  #define PMU_PMCR_X         (1 << 4)
> > >  #define PMU_PMCR_DP        (1 << 5)
> > >  #define PMU_PMCR_LC        (1 << 6)
> > > +#define PMU_PMCR_LP        (1 << 7)
> > >  #define PMU_PMCR_N_SHIFT   11
> > >  #define PMU_PMCR_N_MASK    0x1f
> > >  #define PMU_PMCR_ID_SHIFT  16
> > > @@ -55,10 +56,15 @@
> > >  #define EXT_COMMON_EVENTS_HIGH	0x403F
> > >  
> > >  #define ALL_SET			0x00000000FFFFFFFFULL
> > > +#define ALL_SET_64		0xFFFFFFFFFFFFFFFFULL
> > >  #define ALL_CLEAR		0x0000000000000000ULL
> > >  #define PRE_OVERFLOW		0x00000000FFFFFFF0ULL
> > > +#define PRE_OVERFLOW_64		0xFFFFFFFFFFFFFFF0ULL
> > >  #define PRE_OVERFLOW2		0x00000000FFFFFFDCULL
> > >  
> > > +#define PRE_OVERFLOW_AT(_64b)	(_64b ? PRE_OVERFLOW_64 : PRE_OVERFLOW)
> > > +#define ALL_SET_AT(_64b)	(_64b ? ALL_SET_64 : ALL_SET)
> > > +
> > >  #define PMU_PPI			23
> > >  
> > >  struct pmu {
> > > @@ -429,8 +435,10 @@ static bool satisfy_prerequisites(uint32_t *events, unsigned int nb_events,
> > >  static void test_basic_event_count(bool overflow_at_64bits)
> > >  {
> > >  	uint32_t implemented_counter_mask, non_implemented_counter_mask;
> > > -	uint32_t counter_mask;
> > > +	uint64_t pre_overflow = PRE_OVERFLOW_AT(overflow_at_64bits);
> > > +	uint64_t pmcr_lp = overflow_at_64bits ? PMU_PMCR_LP : 0;
> > >  	uint32_t events[] = {CPU_CYCLES, INST_RETIRED};
> > > +	uint32_t counter_mask;
> > >  
> > >  	if (!satisfy_prerequisites(events, ARRAY_SIZE(events),
> > >  				   overflow_at_64bits))
> > > @@ -452,13 +460,13 @@ static void test_basic_event_count(bool overflow_at_64bits)
> > >  	 * 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);
> > > +	set_pmcr(pmu.pmcr_ro | PMU_PMCR_LC | PMU_PMCR_C | PMU_PMCR_P | pmcr_lp);
> > >  	isb();
> > > -	report(get_pmcr() == (pmu.pmcr_ro | PMU_PMCR_LC), "pmcr: reset counters");
> > > +	report(get_pmcr() == (pmu.pmcr_ro | PMU_PMCR_LC | pmcr_lp), "pmcr: reset counters");
> > >  
> > >  	/* Preset counter #0 to pre overflow value to trigger an overflow */
> > > -	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW);
> > > -	report(read_regn_el0(pmevcntr, 0) == PRE_OVERFLOW,
> > > +	write_regn_el0(pmevcntr, 0, pre_overflow);
> > > +	report(read_regn_el0(pmevcntr, 0) == pre_overflow,
> > >  		"counter #0 preset to pre-overflow value");
> > >  	report(!read_regn_el0(pmevcntr, 1), "counter #1 is 0");
> > >  
> > > @@ -511,6 +519,8 @@ static void test_mem_access(bool overflow_at_64bits)
> > >  {
> > >  	void *addr = malloc(PAGE_SIZE);
> > >  	uint32_t events[] = {MEM_ACCESS, MEM_ACCESS};
> > > +	uint64_t pre_overflow = PRE_OVERFLOW_AT(overflow_at_64bits);
> > > +	uint64_t pmcr_lp = overflow_at_64bits ? PMU_PMCR_LP : 0;
> > >  
> > >  	if (!satisfy_prerequisites(events, ARRAY_SIZE(events),
> > >  				   overflow_at_64bits))
> > > @@ -522,7 +532,7 @@ static void test_mem_access(bool overflow_at_64bits)
> > >  	write_regn_el0(pmevtyper, 1, MEM_ACCESS | PMEVTYPER_EXCLUDE_EL0);
> > >  	write_sysreg_s(0x3, PMCNTENSET_EL0);
> > >  	isb();
> > > -	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E);
> > > +	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E | pmcr_lp);
> > >  	report_info("counter #0 is %ld (MEM_ACCESS)", read_regn_el0(pmevcntr, 0));
> > >  	report_info("counter #1 is %ld (MEM_ACCESS)", read_regn_el0(pmevcntr, 1));
> > >  	/* We may measure more than 20 mem access depending on the core */
> > > @@ -532,11 +542,11 @@ static void test_mem_access(bool overflow_at_64bits)
> > >  
> > >  	pmu_reset();
> > >  
> > > -	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW);
> > > -	write_regn_el0(pmevcntr, 1, PRE_OVERFLOW);
> > > +	write_regn_el0(pmevcntr, 0, pre_overflow);
> > > +	write_regn_el0(pmevcntr, 1, pre_overflow);
> > >  	write_sysreg_s(0x3, PMCNTENSET_EL0);
> > >  	isb();
> > > -	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E);
> > > +	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E | pmcr_lp);
> > >  	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",
> > > @@ -546,6 +556,8 @@ static void test_mem_access(bool overflow_at_64bits)
> > >  
> > >  static void test_sw_incr(bool overflow_at_64bits)
> > >  {
> > > +	uint64_t pre_overflow = PRE_OVERFLOW_AT(overflow_at_64bits);
> > > +	uint64_t pmcr_lp = overflow_at_64bits ? PMU_PMCR_LP : 0;
> > >  	uint32_t events[] = {SW_INCR, SW_INCR};
> > >  	uint64_t cntr0;
> > >  	int i;
> > > @@ -561,7 +573,7 @@ static void test_sw_incr(bool overflow_at_64bits)
> > >  	/* enable counters #0 and #1 */
> > >  	write_sysreg_s(0x3, PMCNTENSET_EL0);
> > >  
> > > -	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW);
> > > +	write_regn_el0(pmevcntr, 0, pre_overflow);
> > >  	isb();
> > >  
> > >  	for (i = 0; i < 100; i++)
> > > @@ -569,21 +581,21 @@ static void test_sw_incr(bool overflow_at_64bits)
> > >  
> > >  	isb();
> > >  	report_info("SW_INCR counter #0 has value %ld", read_regn_el0(pmevcntr, 0));
> > > -	report(read_regn_el0(pmevcntr, 0) == PRE_OVERFLOW,
> > > +	report(read_regn_el0(pmevcntr, 0) == pre_overflow,
> > >  		"PWSYNC does not increment if PMCR.E is unset");
> > >  
> > >  	pmu_reset();
> > >  
> > > -	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW);
> > > +	write_regn_el0(pmevcntr, 0, pre_overflow);
> > >  	write_sysreg_s(0x3, PMCNTENSET_EL0);
> > > -	set_pmcr(pmu.pmcr_ro | PMU_PMCR_E);
> > > +	set_pmcr(pmu.pmcr_ro | PMU_PMCR_E | pmcr_lp);
> > >  	isb();
> > >  
> > >  	for (i = 0; i < 100; i++)
> > >  		write_sysreg(0x3, pmswinc_el0);
> > >  
> > >  	isb();
> > > -	cntr0 = (pmu.version < ID_DFR0_PMU_V3_8_5) ? 84 : PRE_OVERFLOW + 100;
> > > +	cntr0 = (pmu.version < ID_DFR0_PMU_V3_8_5) ? 84 : pre_overflow + 100;
> > >  	report(read_regn_el0(pmevcntr, 0) == cntr0, "counter #0 after + 100 SW_INCR");
> > >  	report(read_regn_el0(pmevcntr, 1) == 100, "counter #1 after + 100 SW_INCR");
> > >  	report_info("counter values after 100 SW_INCR #0=%ld #1=%ld",
> > > @@ -844,6 +856,9 @@ static bool expect_interrupts(uint32_t bitmap)
> > >  
> > >  static void test_overflow_interrupt(bool overflow_at_64bits)
> > >  {
> > > +	uint64_t pre_overflow = PRE_OVERFLOW_AT(overflow_at_64bits);
> > > +	uint64_t all_set = ALL_SET_AT(overflow_at_64bits);
> > > +	uint64_t pmcr_lp = overflow_at_64bits ? PMU_PMCR_LP : 0;
> > >  	uint32_t events[] = {MEM_ACCESS, SW_INCR};
> > >  	void *addr = malloc(PAGE_SIZE);
> > >  	int i;
> > > @@ -862,16 +877,16 @@ static void test_overflow_interrupt(bool overflow_at_64bits)
> > >  	write_regn_el0(pmevtyper, 0, MEM_ACCESS | PMEVTYPER_EXCLUDE_EL0);
> > >  	write_regn_el0(pmevtyper, 1, SW_INCR | PMEVTYPER_EXCLUDE_EL0);
> > >  	write_sysreg_s(0x3, PMCNTENSET_EL0);
> > > -	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW);
> > > -	write_regn_el0(pmevcntr, 1, PRE_OVERFLOW);
> > > +	write_regn_el0(pmevcntr, 0, pre_overflow);
> > > +	write_regn_el0(pmevcntr, 1, pre_overflow);
> > >  	isb();
> > >  
> > >  	/* interrupts are disabled (PMINTENSET_EL1 == 0) */
> > >  
> > > -	mem_access_loop(addr, 200, pmu.pmcr_ro | PMU_PMCR_E);
> > > +	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E | pmcr_lp);
> > >  	report(expect_interrupts(0), "no overflow interrupt after preset");
> > >  
> > > -	set_pmcr(pmu.pmcr_ro | PMU_PMCR_E);
> > > +	set_pmcr(pmu.pmcr_ro | PMU_PMCR_E | pmcr_lp);
> > >  	isb();
> > >  
> > >  	for (i = 0; i < 100; i++)
> > > @@ -886,12 +901,12 @@ static void test_overflow_interrupt(bool overflow_at_64bits)
> > >  
> > >  	pmu_reset_stats();
> > >  
> > > -	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW);
> > > -	write_regn_el0(pmevcntr, 1, PRE_OVERFLOW);
> > > +	write_regn_el0(pmevcntr, 0, pre_overflow);
> > > +	write_regn_el0(pmevcntr, 1, pre_overflow);
> > >  	write_sysreg(ALL_SET, pmintenset_el1);
> > >  	isb();
> > >  
> > > -	mem_access_loop(addr, 200, pmu.pmcr_ro | PMU_PMCR_E);
> > > +	mem_access_loop(addr, 200, pmu.pmcr_ro | PMU_PMCR_E | pmcr_lp);
> > >  	for (i = 0; i < 100; i++)
> > >  		write_sysreg(0x3, pmswinc_el0);
> > >  
> > > @@ -900,25 +915,35 @@ static void test_overflow_interrupt(bool overflow_at_64bits)
> > >  	report(expect_interrupts(0x3),
> > >  		"overflow interrupts expected on #0 and #1");
> > >  
> > > -	/* promote to 64-b */
> > > +	/*
> > > +	 * promote to 64-b:
> > > +	 *
> > > +	 * This only applies to the !overflow_at_64bits case, as
> > > +	 * overflow_at_64bits doesn't implement CHAIN events. The
> > > +	 * overflow_at_64bits case just checks that chained counters are
> > > +	 * not incremented when PMCR.LP == 1.
> > > +	 */
> > 
> > If this doesn't do anything for when overflow_at_64bits, and since the
> > interrupt is already tested before this part, why not exit early?
> > 
> > Or the test could check that the CHAIN event indeed does not increment when
> > LP=1 at the end of this function.
> 
> That's precisely what it's doing.

Where? Are you referring to this part?

	if (overflow_at_64bits)
		report(expect_interrupts(0x1),
		       "expect overflow interrupt on even counter");

My suggestion was to check that the counter that counts the CHAIN event
didn't increment (PMEVCNTR1_EL0 == all_set). I was thinking that since KVM
emulates the CHAIN event in software, it might be useful to check that the
final state of all registers is consistent. I'll leave it up to you to
decide if you want to add this extra check.

Thanks,
Alex

> 
> Thanks!
> Ricardo
> 
> > 
> > Thanks,
> > Alex
> > 
> > >  
> > >  	pmu_reset_stats();
> > >  
> > >  	write_regn_el0(pmevtyper, 1, CHAIN | PMEVTYPER_EXCLUDE_EL0);
> > > -	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW);
> > > +	write_regn_el0(pmevcntr, 0, pre_overflow);
> > >  	isb();
> > > -	mem_access_loop(addr, 200, pmu.pmcr_ro | PMU_PMCR_E);
> > > -	report(expect_interrupts(0x1),
> > > -		"expect overflow interrupt on 32b boundary");
> > > +	mem_access_loop(addr, 200, pmu.pmcr_ro | PMU_PMCR_E | pmcr_lp);
> > > +	report(expect_interrupts(0x1), "expect overflow interrupt");
> > >  
> > >  	/* overflow on odd counter */
> > >  	pmu_reset_stats();
> > > -	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW);
> > > -	write_regn_el0(pmevcntr, 1, ALL_SET);
> > > +	write_regn_el0(pmevcntr, 0, pre_overflow);
> > > +	write_regn_el0(pmevcntr, 1, all_set);
> > >  	isb();
> > > -	mem_access_loop(addr, 400, pmu.pmcr_ro | PMU_PMCR_E);
> > > -	report(expect_interrupts(0x3),
> > > -		"expect overflow interrupt on even and odd counter");
> > > +	mem_access_loop(addr, 400, pmu.pmcr_ro | PMU_PMCR_E | pmcr_lp);
> > > +	if (overflow_at_64bits)
> > > +		report(expect_interrupts(0x1),
> > > +		       "expect overflow interrupt on even counter");
> > > +	else
> > > +		report(expect_interrupts(0x3),
> > > +		       "expect overflow interrupt on even and odd counter");
> > >  }
> > >  #endif
> > >  
> > > @@ -1119,10 +1144,13 @@ int main(int argc, char *argv[])
> > >  		report_prefix_pop();
> > >  	} else if (strcmp(argv[1], "pmu-basic-event-count") == 0) {
> > >  		run_test(argv[1], test_basic_event_count, false);
> > > +		run_test(argv[1], test_basic_event_count, true);
> > >  	} else if (strcmp(argv[1], "pmu-mem-access") == 0) {
> > >  		run_test(argv[1], test_mem_access, false);
> > > +		run_test(argv[1], test_mem_access, true);
> > >  	} else if (strcmp(argv[1], "pmu-sw-incr") == 0) {
> > >  		run_test(argv[1], test_sw_incr, false);
> > > +		run_test(argv[1], test_sw_incr, true);
> > >  	} else if (strcmp(argv[1], "pmu-chained-counters") == 0) {
> > >  		run_test(argv[1], test_chained_counters, false);
> > >  	} else if (strcmp(argv[1], "pmu-chained-sw-incr") == 0) {
> > > @@ -1131,6 +1159,7 @@ int main(int argc, char *argv[])
> > >  		run_test(argv[1], test_chain_promotion, false);
> > >  	} else if (strcmp(argv[1], "pmu-overflow-interrupt") == 0) {
> > >  		run_test(argv[1], test_overflow_interrupt, false);
> > > +		run_test(argv[1], test_overflow_interrupt, true);
> > >  	} else {
> > >  		report_abort("Unknown sub-test '%s'", argv[1]);
> > >  	}
> > > -- 
> > > 2.39.0.rc0.267.gcb52ba06e7-goog
> > > 

Re: [kvm-unit-tests PATCH 3/3] arm: pmu: Add tests for 64-bit overflows

[Ricardo Koller]

On Wed, Dec 14, 2022 at 10:45:07AM +0000, Alexandru Elisei wrote:
> Hi,
> 
> On Tue, Dec 13, 2022 at 10:04:04AM -0800, Ricardo Koller wrote:
> > On Tue, Dec 13, 2022 at 05:03:40PM +0000, Alexandru Elisei wrote:
> > > Hi,
> > > 
> > > Checked that all places where ALL_SET/PRE_OVERFLOW were used are now taking
> > > into account the fact that counters are programmed to be 64bit.
> > > 
> > > In the case of 64bit counters, the printf format specifier is %ld, which
> > > means that ALL_SET_64 and PRE_OVERFLOW_64 are now displayed as negative
> > > numbers. For example:
> > > 
> > > INFO: pmu: pmu-sw-incr: 32-bit: SW_INCR counter #0 has value 4294967280
> > > PASS: pmu: pmu-sw-incr: 32-bit: PWSYNC does not increment if PMCR.E is unset
> > > [..]
> > > INFO: pmu: pmu-sw-incr: 64-bit: SW_INCR counter #0 has value -16
> > > PASS: pmu: pmu-sw-incr: 64-bit: PWSYNC does not increment if PMCR.E is unset
> > > 
> > 
> > Argh, I didn't notice this. I think this would be more useful if it
> > printed %llx in all cases.
> 
> Indeed, printing the hex value will make it easier to match it against the
> defines.
> 
> One more comment below.
> 
> > 
> > > I was thinking that the format specifiers could be changed to unsigned
> > > long.  The counters only increment, they don't decrement, and I can't think
> > > how printing them as signed could be useful.
> > > 
> > > One more comment below.
> > > 
> > > On Fri, Dec 02, 2022 at 04:55:27AM +0000, Ricardo Koller wrote:
> > > > Modify all tests checking overflows to support both 32 (PMCR_EL0.LP == 0)
> > > > and 64-bit overflows (PMCR_EL0.LP == 1). 64-bit overflows are only
> > > > supported on PMUv3p5.
> > > > 
> > > > Note that chained tests do not implement "overflow_at_64bits == true".
> > > > That's because there are no CHAIN events when "PMCR_EL0.LP == 1" (for more
> > > > details see AArch64.IncrementEventCounter() pseudocode in the ARM ARM DDI
> > > > 0487H.a, J1.1.1 "aarch64/debug").
> > > > 
> > > > Signed-off-by: Ricardo Koller <ricarkol@google.com>
> > > > ---
> > > >  arm/pmu.c | 91 ++++++++++++++++++++++++++++++++++++-------------------
> > > >  1 file changed, 60 insertions(+), 31 deletions(-)
> > > > 
> > > > diff --git a/arm/pmu.c b/arm/pmu.c
> > > > index 59e5bfe..3cb563b 100644
> > > > --- a/arm/pmu.c
> > > > +++ b/arm/pmu.c
> > > > @@ -28,6 +28,7 @@
> > > >  #define PMU_PMCR_X         (1 << 4)
> > > >  #define PMU_PMCR_DP        (1 << 5)
> > > >  #define PMU_PMCR_LC        (1 << 6)
> > > > +#define PMU_PMCR_LP        (1 << 7)
> > > >  #define PMU_PMCR_N_SHIFT   11
> > > >  #define PMU_PMCR_N_MASK    0x1f
> > > >  #define PMU_PMCR_ID_SHIFT  16
> > > > @@ -55,10 +56,15 @@
> > > >  #define EXT_COMMON_EVENTS_HIGH	0x403F
> > > >  
> > > >  #define ALL_SET			0x00000000FFFFFFFFULL
> > > > +#define ALL_SET_64		0xFFFFFFFFFFFFFFFFULL
> > > >  #define ALL_CLEAR		0x0000000000000000ULL
> > > >  #define PRE_OVERFLOW		0x00000000FFFFFFF0ULL
> > > > +#define PRE_OVERFLOW_64		0xFFFFFFFFFFFFFFF0ULL
> > > >  #define PRE_OVERFLOW2		0x00000000FFFFFFDCULL
> > > >  
> > > > +#define PRE_OVERFLOW_AT(_64b)	(_64b ? PRE_OVERFLOW_64 : PRE_OVERFLOW)
> > > > +#define ALL_SET_AT(_64b)	(_64b ? ALL_SET_64 : ALL_SET)
> > > > +
> > > >  #define PMU_PPI			23
> > > >  
> > > >  struct pmu {
> > > > @@ -429,8 +435,10 @@ static bool satisfy_prerequisites(uint32_t *events, unsigned int nb_events,
> > > >  static void test_basic_event_count(bool overflow_at_64bits)
> > > >  {
> > > >  	uint32_t implemented_counter_mask, non_implemented_counter_mask;
> > > > -	uint32_t counter_mask;
> > > > +	uint64_t pre_overflow = PRE_OVERFLOW_AT(overflow_at_64bits);
> > > > +	uint64_t pmcr_lp = overflow_at_64bits ? PMU_PMCR_LP : 0;
> > > >  	uint32_t events[] = {CPU_CYCLES, INST_RETIRED};
> > > > +	uint32_t counter_mask;
> > > >  
> > > >  	if (!satisfy_prerequisites(events, ARRAY_SIZE(events),
> > > >  				   overflow_at_64bits))
> > > > @@ -452,13 +460,13 @@ static void test_basic_event_count(bool overflow_at_64bits)
> > > >  	 * 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);
> > > > +	set_pmcr(pmu.pmcr_ro | PMU_PMCR_LC | PMU_PMCR_C | PMU_PMCR_P | pmcr_lp);
> > > >  	isb();
> > > > -	report(get_pmcr() == (pmu.pmcr_ro | PMU_PMCR_LC), "pmcr: reset counters");
> > > > +	report(get_pmcr() == (pmu.pmcr_ro | PMU_PMCR_LC | pmcr_lp), "pmcr: reset counters");
> > > >  
> > > >  	/* Preset counter #0 to pre overflow value to trigger an overflow */
> > > > -	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW);
> > > > -	report(read_regn_el0(pmevcntr, 0) == PRE_OVERFLOW,
> > > > +	write_regn_el0(pmevcntr, 0, pre_overflow);
> > > > +	report(read_regn_el0(pmevcntr, 0) == pre_overflow,
> > > >  		"counter #0 preset to pre-overflow value");
> > > >  	report(!read_regn_el0(pmevcntr, 1), "counter #1 is 0");
> > > >  
> > > > @@ -511,6 +519,8 @@ static void test_mem_access(bool overflow_at_64bits)
> > > >  {
> > > >  	void *addr = malloc(PAGE_SIZE);
> > > >  	uint32_t events[] = {MEM_ACCESS, MEM_ACCESS};
> > > > +	uint64_t pre_overflow = PRE_OVERFLOW_AT(overflow_at_64bits);
> > > > +	uint64_t pmcr_lp = overflow_at_64bits ? PMU_PMCR_LP : 0;
> > > >  
> > > >  	if (!satisfy_prerequisites(events, ARRAY_SIZE(events),
> > > >  				   overflow_at_64bits))
> > > > @@ -522,7 +532,7 @@ static void test_mem_access(bool overflow_at_64bits)
> > > >  	write_regn_el0(pmevtyper, 1, MEM_ACCESS | PMEVTYPER_EXCLUDE_EL0);
> > > >  	write_sysreg_s(0x3, PMCNTENSET_EL0);
> > > >  	isb();
> > > > -	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E);
> > > > +	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E | pmcr_lp);
> > > >  	report_info("counter #0 is %ld (MEM_ACCESS)", read_regn_el0(pmevcntr, 0));
> > > >  	report_info("counter #1 is %ld (MEM_ACCESS)", read_regn_el0(pmevcntr, 1));
> > > >  	/* We may measure more than 20 mem access depending on the core */
> > > > @@ -532,11 +542,11 @@ static void test_mem_access(bool overflow_at_64bits)
> > > >  
> > > >  	pmu_reset();
> > > >  
> > > > -	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW);
> > > > -	write_regn_el0(pmevcntr, 1, PRE_OVERFLOW);
> > > > +	write_regn_el0(pmevcntr, 0, pre_overflow);
> > > > +	write_regn_el0(pmevcntr, 1, pre_overflow);
> > > >  	write_sysreg_s(0x3, PMCNTENSET_EL0);
> > > >  	isb();
> > > > -	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E);
> > > > +	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E | pmcr_lp);
> > > >  	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",
> > > > @@ -546,6 +556,8 @@ static void test_mem_access(bool overflow_at_64bits)
> > > >  
> > > >  static void test_sw_incr(bool overflow_at_64bits)
> > > >  {
> > > > +	uint64_t pre_overflow = PRE_OVERFLOW_AT(overflow_at_64bits);
> > > > +	uint64_t pmcr_lp = overflow_at_64bits ? PMU_PMCR_LP : 0;
> > > >  	uint32_t events[] = {SW_INCR, SW_INCR};
> > > >  	uint64_t cntr0;
> > > >  	int i;
> > > > @@ -561,7 +573,7 @@ static void test_sw_incr(bool overflow_at_64bits)
> > > >  	/* enable counters #0 and #1 */
> > > >  	write_sysreg_s(0x3, PMCNTENSET_EL0);
> > > >  
> > > > -	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW);
> > > > +	write_regn_el0(pmevcntr, 0, pre_overflow);
> > > >  	isb();
> > > >  
> > > >  	for (i = 0; i < 100; i++)
> > > > @@ -569,21 +581,21 @@ static void test_sw_incr(bool overflow_at_64bits)
> > > >  
> > > >  	isb();
> > > >  	report_info("SW_INCR counter #0 has value %ld", read_regn_el0(pmevcntr, 0));
> > > > -	report(read_regn_el0(pmevcntr, 0) == PRE_OVERFLOW,
> > > > +	report(read_regn_el0(pmevcntr, 0) == pre_overflow,
> > > >  		"PWSYNC does not increment if PMCR.E is unset");
> > > >  
> > > >  	pmu_reset();
> > > >  
> > > > -	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW);
> > > > +	write_regn_el0(pmevcntr, 0, pre_overflow);
> > > >  	write_sysreg_s(0x3, PMCNTENSET_EL0);
> > > > -	set_pmcr(pmu.pmcr_ro | PMU_PMCR_E);
> > > > +	set_pmcr(pmu.pmcr_ro | PMU_PMCR_E | pmcr_lp);
> > > >  	isb();
> > > >  
> > > >  	for (i = 0; i < 100; i++)
> > > >  		write_sysreg(0x3, pmswinc_el0);
> > > >  
> > > >  	isb();
> > > > -	cntr0 = (pmu.version < ID_DFR0_PMU_V3_8_5) ? 84 : PRE_OVERFLOW + 100;
> > > > +	cntr0 = (pmu.version < ID_DFR0_PMU_V3_8_5) ? 84 : pre_overflow + 100;
> > > >  	report(read_regn_el0(pmevcntr, 0) == cntr0, "counter #0 after + 100 SW_INCR");
> > > >  	report(read_regn_el0(pmevcntr, 1) == 100, "counter #1 after + 100 SW_INCR");
> > > >  	report_info("counter values after 100 SW_INCR #0=%ld #1=%ld",
> > > > @@ -844,6 +856,9 @@ static bool expect_interrupts(uint32_t bitmap)
> > > >  
> > > >  static void test_overflow_interrupt(bool overflow_at_64bits)
> > > >  {
> > > > +	uint64_t pre_overflow = PRE_OVERFLOW_AT(overflow_at_64bits);
> > > > +	uint64_t all_set = ALL_SET_AT(overflow_at_64bits);
> > > > +	uint64_t pmcr_lp = overflow_at_64bits ? PMU_PMCR_LP : 0;
> > > >  	uint32_t events[] = {MEM_ACCESS, SW_INCR};
> > > >  	void *addr = malloc(PAGE_SIZE);
> > > >  	int i;
> > > > @@ -862,16 +877,16 @@ static void test_overflow_interrupt(bool overflow_at_64bits)
> > > >  	write_regn_el0(pmevtyper, 0, MEM_ACCESS | PMEVTYPER_EXCLUDE_EL0);
> > > >  	write_regn_el0(pmevtyper, 1, SW_INCR | PMEVTYPER_EXCLUDE_EL0);
> > > >  	write_sysreg_s(0x3, PMCNTENSET_EL0);
> > > > -	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW);
> > > > -	write_regn_el0(pmevcntr, 1, PRE_OVERFLOW);
> > > > +	write_regn_el0(pmevcntr, 0, pre_overflow);
> > > > +	write_regn_el0(pmevcntr, 1, pre_overflow);
> > > >  	isb();
> > > >  
> > > >  	/* interrupts are disabled (PMINTENSET_EL1 == 0) */
> > > >  
> > > > -	mem_access_loop(addr, 200, pmu.pmcr_ro | PMU_PMCR_E);
> > > > +	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E | pmcr_lp);
> > > >  	report(expect_interrupts(0), "no overflow interrupt after preset");
> > > >  
> > > > -	set_pmcr(pmu.pmcr_ro | PMU_PMCR_E);
> > > > +	set_pmcr(pmu.pmcr_ro | PMU_PMCR_E | pmcr_lp);
> > > >  	isb();
> > > >  
> > > >  	for (i = 0; i < 100; i++)
> > > > @@ -886,12 +901,12 @@ static void test_overflow_interrupt(bool overflow_at_64bits)
> > > >  
> > > >  	pmu_reset_stats();
> > > >  
> > > > -	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW);
> > > > -	write_regn_el0(pmevcntr, 1, PRE_OVERFLOW);
> > > > +	write_regn_el0(pmevcntr, 0, pre_overflow);
> > > > +	write_regn_el0(pmevcntr, 1, pre_overflow);
> > > >  	write_sysreg(ALL_SET, pmintenset_el1);
> > > >  	isb();
> > > >  
> > > > -	mem_access_loop(addr, 200, pmu.pmcr_ro | PMU_PMCR_E);
> > > > +	mem_access_loop(addr, 200, pmu.pmcr_ro | PMU_PMCR_E | pmcr_lp);
> > > >  	for (i = 0; i < 100; i++)
> > > >  		write_sysreg(0x3, pmswinc_el0);
> > > >  
> > > > @@ -900,25 +915,35 @@ static void test_overflow_interrupt(bool overflow_at_64bits)
> > > >  	report(expect_interrupts(0x3),
> > > >  		"overflow interrupts expected on #0 and #1");
> > > >  
> > > > -	/* promote to 64-b */
> > > > +	/*
> > > > +	 * promote to 64-b:
> > > > +	 *
> > > > +	 * This only applies to the !overflow_at_64bits case, as
> > > > +	 * overflow_at_64bits doesn't implement CHAIN events. The
> > > > +	 * overflow_at_64bits case just checks that chained counters are
> > > > +	 * not incremented when PMCR.LP == 1.
> > > > +	 */
> > > 
> > > If this doesn't do anything for when overflow_at_64bits, and since the
> > > interrupt is already tested before this part, why not exit early?
> > > 
> > > Or the test could check that the CHAIN event indeed does not increment when
> > > LP=1 at the end of this function.
> > 
> > That's precisely what it's doing.
> 
> Where? Are you referring to this part?
> 
> 	if (overflow_at_64bits)
> 		report(expect_interrupts(0x1),
> 		       "expect overflow interrupt on even counter");
> 
> My suggestion was to check that the counter that counts the CHAIN event
> didn't increment (PMEVCNTR1_EL0 == all_set).

Ah, I see. Yes, this could check that there are no interrupts and that
the even counter doesn't increment.

> I was thinking that since KVM
> emulates the CHAIN event in software, it might be useful to check that the
> final state of all registers is consistent. I'll leave it up to you to
> decide if you want to add this extra check.

Will add it if no other test is checking that, which I think is the
case.

Thanks!
Ricardo

> 
> Thanks,
> Alex
> 
> > 
> > Thanks!
> > Ricardo
> > 
> > > 
> > > Thanks,
> > > Alex
> > > 
> > > >  
> > > >  	pmu_reset_stats();
> > > >  
> > > >  	write_regn_el0(pmevtyper, 1, CHAIN | PMEVTYPER_EXCLUDE_EL0);
> > > > -	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW);
> > > > +	write_regn_el0(pmevcntr, 0, pre_overflow);
> > > >  	isb();
> > > > -	mem_access_loop(addr, 200, pmu.pmcr_ro | PMU_PMCR_E);
> > > > -	report(expect_interrupts(0x1),
> > > > -		"expect overflow interrupt on 32b boundary");
> > > > +	mem_access_loop(addr, 200, pmu.pmcr_ro | PMU_PMCR_E | pmcr_lp);
> > > > +	report(expect_interrupts(0x1), "expect overflow interrupt");
> > > >  
> > > >  	/* overflow on odd counter */
> > > >  	pmu_reset_stats();
> > > > -	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW);
> > > > -	write_regn_el0(pmevcntr, 1, ALL_SET);
> > > > +	write_regn_el0(pmevcntr, 0, pre_overflow);
> > > > +	write_regn_el0(pmevcntr, 1, all_set);
> > > >  	isb();
> > > > -	mem_access_loop(addr, 400, pmu.pmcr_ro | PMU_PMCR_E);
> > > > -	report(expect_interrupts(0x3),
> > > > -		"expect overflow interrupt on even and odd counter");
> > > > +	mem_access_loop(addr, 400, pmu.pmcr_ro | PMU_PMCR_E | pmcr_lp);
> > > > +	if (overflow_at_64bits)
> > > > +		report(expect_interrupts(0x1),
> > > > +		       "expect overflow interrupt on even counter");
> > > > +	else
> > > > +		report(expect_interrupts(0x3),
> > > > +		       "expect overflow interrupt on even and odd counter");
> > > >  }
> > > >  #endif
> > > >  
> > > > @@ -1119,10 +1144,13 @@ int main(int argc, char *argv[])
> > > >  		report_prefix_pop();
> > > >  	} else if (strcmp(argv[1], "pmu-basic-event-count") == 0) {
> > > >  		run_test(argv[1], test_basic_event_count, false);
> > > > +		run_test(argv[1], test_basic_event_count, true);
> > > >  	} else if (strcmp(argv[1], "pmu-mem-access") == 0) {
> > > >  		run_test(argv[1], test_mem_access, false);
> > > > +		run_test(argv[1], test_mem_access, true);
> > > >  	} else if (strcmp(argv[1], "pmu-sw-incr") == 0) {
> > > >  		run_test(argv[1], test_sw_incr, false);
> > > > +		run_test(argv[1], test_sw_incr, true);
> > > >  	} else if (strcmp(argv[1], "pmu-chained-counters") == 0) {
> > > >  		run_test(argv[1], test_chained_counters, false);
> > > >  	} else if (strcmp(argv[1], "pmu-chained-sw-incr") == 0) {
> > > > @@ -1131,6 +1159,7 @@ int main(int argc, char *argv[])
> > > >  		run_test(argv[1], test_chain_promotion, false);
> > > >  	} else if (strcmp(argv[1], "pmu-overflow-interrupt") == 0) {
> > > >  		run_test(argv[1], test_overflow_interrupt, false);
> > > > +		run_test(argv[1], test_overflow_interrupt, true);
> > > >  	} else {
> > > >  		report_abort("Unknown sub-test '%s'", argv[1]);
> > > >  	}
> > > > -- 
> > > > 2.39.0.rc0.267.gcb52ba06e7-goog
> > > >