[kvm-unit-tests PATCH v2 0/2] arm: pmu-overflow-interrupt: Fix failures on Amberwing

[kvm-unit-tests PATCH v2 0/2] arm: pmu-overflow-interrupt: Fix failures on Amberwing

[Eric Auger]

On Qualcomm Amberwing, some pmu-overflow-interrupt failures can be observed.
Although the even counter overflows, the interrupt is not seen as
expected on guest side. This happens in the subtest after "promote to 64-b"
comment.

After analysis, the PMU overflow interrupt actually hits, ie.
kvm_pmu_perf_overflow() gets called and KVM_REQ_IRQ_PENDING is set,
as expected. However the PMCR.E is reset by the handle_exit path, at
kvm_pmu_handle_pmcr() before the next guest entry and
kvm_pmu_flush_hwstate/kvm_pmu_update_state subsequent call.
There, since the enable bit has been reset, kvm_pmu_update_state() does
not inject the interrupt into the guest.

This does not seem to be a KVM bug but rather an unfortunate
scenario where the test disables the PMCR.E too closely to the
advent of the overflow interrupt.

Since it looks like a benign and inlikely case, let's resize the number
of iterations to prevent the PMCR enable bit from being resetted
immediately at the same time as the actual overflow event.

Also make pmu_stats volatile to prevent any optimizations.

Eric Auger (2):
  arm: pmu: Declare pmu_stats as volatile
  arm: pmu-overflow-interrupt: Increase count values

 arm/pmu.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

-- 
2.41.0

[kvm-unit-tests PATCH v2 1/2] arm: pmu: Declare pmu_stats as volatile

[Eric Auger]

Declare pmu_stats as volatile in order to prevent the compiler
from caching previously read values. This actually fixes
pmu-overflow-interrupt failures on some HW.

Reported-by: Alexandru Elisei <alexandru.elisei@arm.com>
Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 arm/pmu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arm/pmu.c b/arm/pmu.c
index a91a7b1f..86199577 100644
--- a/arm/pmu.c
+++ b/arm/pmu.c
@@ -328,7 +328,7 @@ asm volatile(
 	: "x9", "x10", "cc");
 }
 
-static struct pmu_stats pmu_stats;
+static volatile struct pmu_stats pmu_stats;
 
 static void irq_handler(struct pt_regs *regs)
 {
-- 
2.41.0

Re: [kvm-unit-tests PATCH v2 1/2] arm: pmu: Declare pmu_stats as volatile

[Shaoqin Huang]

On 11/14/23 01:42, Eric Auger wrote:
> Declare pmu_stats as volatile in order to prevent the compiler
> from caching previously read values. This actually fixes
> pmu-overflow-interrupt failures on some HW.
> 
> Reported-by: Alexandru Elisei <alexandru.elisei@arm.com>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
Reviewed-by: Shaoqin Huang <shahuang@redhat.com>
> ---
>   arm/pmu.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arm/pmu.c b/arm/pmu.c
> index a91a7b1f..86199577 100644
> --- a/arm/pmu.c
> +++ b/arm/pmu.c
> @@ -328,7 +328,7 @@ asm volatile(
>   	: "x9", "x10", "cc");
>   }
>   
> -static struct pmu_stats pmu_stats;
> +static volatile struct pmu_stats pmu_stats;
>   
>   static void irq_handler(struct pt_regs *regs)
>   {

-- 
Shaoqin

Re: [kvm-unit-tests PATCH v2 1/2] arm: pmu: Declare pmu_stats as volatile

[Alexandru Elisei]

Hi,

On Mon, Nov 13, 2023 at 06:42:40PM +0100, Eric Auger wrote:
> Declare pmu_stats as volatile in order to prevent the compiler
> from caching previously read values. This actually fixes
> pmu-overflow-interrupt failures on some HW.

Looks good to me:

Reviewed-by: Alexandru Elisei <alexandru.elisei@arm.com>

Thanks,
Alex

> 
> Reported-by: Alexandru Elisei <alexandru.elisei@arm.com>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> ---
>  arm/pmu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arm/pmu.c b/arm/pmu.c
> index a91a7b1f..86199577 100644
> --- a/arm/pmu.c
> +++ b/arm/pmu.c
> @@ -328,7 +328,7 @@ asm volatile(
>  	: "x9", "x10", "cc");
>  }
>  
> -static struct pmu_stats pmu_stats;
> +static volatile struct pmu_stats pmu_stats;
>  
>  static void irq_handler(struct pt_regs *regs)
>  {
> -- 
> 2.41.0
> 

[kvm-unit-tests PATCH v2 2/2] arm: pmu-overflow-interrupt: Increase count values

[Eric Auger]

On some hardware, some pmu-overflow-interrupt failures can be observed.
Although the even counter overflows, the interrupt is not seen as
expected. This happens in the subtest after "promote to 64-b" comment.
After analysis, the PMU overflow interrupt actually hits, ie.
kvm_pmu_perf_overflow() gets called and KVM_REQ_IRQ_PENDING is set,
as expected. However the PMCR.E is reset by the handle_exit path, at
kvm_pmu_handle_pmcr() before the next guest entry and
kvm_pmu_flush_hwstate/kvm_pmu_update_state subsequent call.
There, since the enable bit has been reset, kvm_pmu_update_state() does
not inject the interrupt into the guest.

This does not seem to be a KVM bug but rather an unfortunate
scenario where the test disables the PMCR.E too closely to the
advent of the overflow interrupt.

Since it looks like a benign and inlikely case, let's resize the number
of iterations to prevent the PMCR enable bit from being resetted
immediately at the same time as the actual overflow event.

COUNT_INT is introduced, arbitrarily set to 1000 iterations and is
used in this test.

An alternative would be to let the PMU enabled and wait for the
interrupt but those extra executions might disturb the counting.

Reported-by: Jan Richter <jarichte@redhat.com>
Signed-off-by: Eric Auger <eric.auger@redhat.com>

---

v1 -> v2:
- Only increase mem_access_loop iterations
---
 arm/pmu.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/arm/pmu.c b/arm/pmu.c
index 86199577..4b388899 100644
--- a/arm/pmu.c
+++ b/arm/pmu.c
@@ -66,6 +66,7 @@
 #define PRE_OVERFLOW_64		0xFFFFFFFFFFFFFFF0ULL
 #define COUNT 250
 #define MARGIN 100
+#define COUNT_INT 1000
 /*
  * PRE_OVERFLOW2 is set so that 1st @COUNT iterations do not
  * produce 32b overflow and 2nd @COUNT iterations do. To accommodate
@@ -978,7 +979,7 @@ static void test_overflow_interrupt(bool overflow_at_64bits)
 
 	/* interrupts are disabled (PMINTENSET_EL1 == 0) */
 
-	mem_access_loop(addr, 200, pmu.pmcr_ro | PMU_PMCR_E | pmcr_lp);
+	mem_access_loop(addr, COUNT_INT, 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 | pmcr_lp);
@@ -1002,7 +1003,7 @@ static void test_overflow_interrupt(bool overflow_at_64bits)
 	write_sysreg(ALL_SET_32, pmintenset_el1);
 	isb();
 
-	mem_access_loop(addr, 200, pmu.pmcr_ro | PMU_PMCR_E | pmcr_lp);
+	mem_access_loop(addr, COUNT_INT, pmu.pmcr_ro | PMU_PMCR_E | pmcr_lp);
 
 	set_pmcr(pmu.pmcr_ro | PMU_PMCR_E | pmcr_lp);
 	isb();
@@ -1010,7 +1011,7 @@ static void test_overflow_interrupt(bool overflow_at_64bits)
 	for (i = 0; i < 100; i++)
 		write_sysreg(0x3, pmswinc_el0);
 
-	mem_access_loop(addr, 200, pmu.pmcr_ro);
+	mem_access_loop(addr, COUNT_INT, pmu.pmcr_ro);
 	report_info("overflow=0x%lx", read_sysreg(pmovsclr_el0));
 	report(expect_interrupts(0x3),
 		"overflow interrupts expected on #0 and #1");
@@ -1029,7 +1030,7 @@ static void test_overflow_interrupt(bool overflow_at_64bits)
 	write_regn_el0(pmevtyper, 1, CHAIN | PMEVTYPER_EXCLUDE_EL0);
 	write_regn_el0(pmevcntr, 0, pre_overflow);
 	isb();
-	mem_access_loop(addr, 200, pmu.pmcr_ro | PMU_PMCR_E | pmcr_lp);
+	mem_access_loop(addr, COUNT_INT, pmu.pmcr_ro | PMU_PMCR_E | pmcr_lp);
 	report(expect_interrupts(0x1), "expect overflow interrupt");
 
 	/* overflow on odd counter */
@@ -1037,7 +1038,7 @@ static void test_overflow_interrupt(bool overflow_at_64bits)
 	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 | pmcr_lp);
+	mem_access_loop(addr, COUNT_INT, pmu.pmcr_ro | PMU_PMCR_E | pmcr_lp);
 	if (overflow_at_64bits) {
 		report(expect_interrupts(0x1),
 		       "expect overflow interrupt on even counter");
-- 
2.41.0

Re: [kvm-unit-tests PATCH v2 2/2] arm: pmu-overflow-interrupt: Increase count values

[Shaoqin Huang]

On 11/14/23 01:42, Eric Auger wrote:
> On some hardware, some pmu-overflow-interrupt failures can be observed.
> Although the even counter overflows, the interrupt is not seen as
> expected. This happens in the subtest after "promote to 64-b" comment.
> After analysis, the PMU overflow interrupt actually hits, ie.
> kvm_pmu_perf_overflow() gets called and KVM_REQ_IRQ_PENDING is set,
> as expected. However the PMCR.E is reset by the handle_exit path, at
> kvm_pmu_handle_pmcr() before the next guest entry and
> kvm_pmu_flush_hwstate/kvm_pmu_update_state subsequent call.
> There, since the enable bit has been reset, kvm_pmu_update_state() does
> not inject the interrupt into the guest.
> 
> This does not seem to be a KVM bug but rather an unfortunate
> scenario where the test disables the PMCR.E too closely to the
> advent of the overflow interrupt.
> 
> Since it looks like a benign and inlikely case, let's resize the number
> of iterations to prevent the PMCR enable bit from being resetted
> immediately at the same time as the actual overflow event.
> 
> COUNT_INT is introduced, arbitrarily set to 1000 iterations and is
> used in this test.
> 
> An alternative would be to let the PMU enabled and wait for the
> interrupt but those extra executions might disturb the counting.
> 
> Reported-by: Jan Richter <jarichte@redhat.com>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
Reviewed-by: Shaoqin Huang <shahuang@redhat.com>
> 
> ---
> 
> v1 -> v2:
> - Only increase mem_access_loop iterations
> ---
>   arm/pmu.c | 11 ++++++-----
>   1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/arm/pmu.c b/arm/pmu.c
> index 86199577..4b388899 100644
> --- a/arm/pmu.c
> +++ b/arm/pmu.c
> @@ -66,6 +66,7 @@
>   #define PRE_OVERFLOW_64		0xFFFFFFFFFFFFFFF0ULL
>   #define COUNT 250
>   #define MARGIN 100
> +#define COUNT_INT 1000
>   /*
>    * PRE_OVERFLOW2 is set so that 1st @COUNT iterations do not
>    * produce 32b overflow and 2nd @COUNT iterations do. To accommodate
> @@ -978,7 +979,7 @@ static void test_overflow_interrupt(bool overflow_at_64bits)
>   
>   	/* interrupts are disabled (PMINTENSET_EL1 == 0) */
>   
> -	mem_access_loop(addr, 200, pmu.pmcr_ro | PMU_PMCR_E | pmcr_lp);
> +	mem_access_loop(addr, COUNT_INT, 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 | pmcr_lp);
> @@ -1002,7 +1003,7 @@ static void test_overflow_interrupt(bool overflow_at_64bits)
>   	write_sysreg(ALL_SET_32, pmintenset_el1);
>   	isb();
>   
> -	mem_access_loop(addr, 200, pmu.pmcr_ro | PMU_PMCR_E | pmcr_lp);
> +	mem_access_loop(addr, COUNT_INT, pmu.pmcr_ro | PMU_PMCR_E | pmcr_lp);
>   
>   	set_pmcr(pmu.pmcr_ro | PMU_PMCR_E | pmcr_lp);
>   	isb();
> @@ -1010,7 +1011,7 @@ static void test_overflow_interrupt(bool overflow_at_64bits)
>   	for (i = 0; i < 100; i++)
>   		write_sysreg(0x3, pmswinc_el0);
>   
> -	mem_access_loop(addr, 200, pmu.pmcr_ro);
> +	mem_access_loop(addr, COUNT_INT, pmu.pmcr_ro);
>   	report_info("overflow=0x%lx", read_sysreg(pmovsclr_el0));
>   	report(expect_interrupts(0x3),
>   		"overflow interrupts expected on #0 and #1");
> @@ -1029,7 +1030,7 @@ static void test_overflow_interrupt(bool overflow_at_64bits)
>   	write_regn_el0(pmevtyper, 1, CHAIN | PMEVTYPER_EXCLUDE_EL0);
>   	write_regn_el0(pmevcntr, 0, pre_overflow);
>   	isb();
> -	mem_access_loop(addr, 200, pmu.pmcr_ro | PMU_PMCR_E | pmcr_lp);
> +	mem_access_loop(addr, COUNT_INT, pmu.pmcr_ro | PMU_PMCR_E | pmcr_lp);
>   	report(expect_interrupts(0x1), "expect overflow interrupt");
>   
>   	/* overflow on odd counter */
> @@ -1037,7 +1038,7 @@ static void test_overflow_interrupt(bool overflow_at_64bits)
>   	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 | pmcr_lp);
> +	mem_access_loop(addr, COUNT_INT, pmu.pmcr_ro | PMU_PMCR_E | pmcr_lp);
>   	if (overflow_at_64bits) {
>   		report(expect_interrupts(0x1),
>   		       "expect overflow interrupt on even counter");

-- 
Shaoqin

Re: [kvm-unit-tests PATCH v2 0/2] arm: pmu-overflow-interrupt: Fix failures on Amberwing

[Andrew Jones]

On Mon, Nov 13, 2023 at 06:42:39PM +0100, Eric Auger wrote:
> On Qualcomm Amberwing, some pmu-overflow-interrupt failures can be observed.
> Although the even counter overflows, the interrupt is not seen as
> expected on guest side. This happens in the subtest after "promote to 64-b"
> comment.
> 
> After analysis, the PMU overflow interrupt actually hits, ie.
> kvm_pmu_perf_overflow() gets called and KVM_REQ_IRQ_PENDING is set,
> as expected. However the PMCR.E is reset by the handle_exit path, at
> kvm_pmu_handle_pmcr() before the next guest entry and
> kvm_pmu_flush_hwstate/kvm_pmu_update_state subsequent call.
> There, since the enable bit has been reset, kvm_pmu_update_state() does
> not inject the interrupt into the guest.
> 
> This does not seem to be a KVM bug but rather an unfortunate
> scenario where the test disables the PMCR.E too closely to the
> advent of the overflow interrupt.
> 
> Since it looks like a benign and inlikely case, let's resize the number
> of iterations to prevent the PMCR enable bit from being resetted
> immediately at the same time as the actual overflow event.
> 
> Also make pmu_stats volatile to prevent any optimizations.
> 
> Eric Auger (2):
>   arm: pmu: Declare pmu_stats as volatile
>   arm: pmu-overflow-interrupt: Increase count values
> 
>  arm/pmu.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> -- 
> 2.41.0
>

Merged into https://gitlab.com/kvm-unit-tests/kvm-unit-tests master

Thanks,
drew