[kvm-unit-tests PATCH] arm64: timer: ignore ISTATUS with disabled timer

[kvm-unit-tests PATCH] arm64: timer: ignore ISTATUS with disabled timer

[Nadav Amit]

From: Nadav Amit <nadav.amit@gmail.com>

According to ARM specifications for the vtimer (CNTV_CTL_EL0): "When the
value of the ENABLE bit is 0, the ISTATUS field is UNKNOWN."

Currently the test, however, does check that ISTATUS is cleared when the
ENABLE bit is zero. Remove this check as the value is unknown.

Signed-off-by: Nadav Amit <nadav.amit@gmail.com>
---
 arm/timer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arm/timer.c b/arm/timer.c
index 2cb8051..0b976a7 100644
--- a/arm/timer.c
+++ b/arm/timer.c
@@ -256,7 +256,7 @@ static void test_timer_pending(struct timer_info *info)
 	set_timer_irq_enabled(info, true);
 
 	report(!info->irq_received, "no interrupt when timer is disabled");
-	report(!timer_pending(info) && gic_timer_check_state(info, GIC_IRQ_STATE_INACTIVE),
+	report(gic_timer_check_state(info, GIC_IRQ_STATE_INACTIVE),
 			"interrupt signal no longer pending");
 
 	info->write_cval(now - 1);
-- 
2.34.1

Re: [kvm-unit-tests PATCH] arm64: timer: ignore ISTATUS with disabled timer

[Andrew Jones]

On Wed, Jun 14, 2023 at 05:38:32PM -0700, Nadav Amit wrote:
> From: Nadav Amit <nadav.amit@gmail.com>
> 
> According to ARM specifications for the vtimer (CNTV_CTL_EL0): "When the
> value of the ENABLE bit is 0, the ISTATUS field is UNKNOWN."
> 
> Currently the test, however, does check that ISTATUS is cleared when the
> ENABLE bit is zero. Remove this check as the value is unknown.

Hmm, does it? timer_pending() is

 /* Check that the timer condition is met. */
 static bool timer_pending(struct timer_info *info)
 {
        return (info->read_ctl() & ARCH_TIMER_CTL_ENABLE) &&
                (info->read_ctl() & ARCH_TIMER_CTL_ISTATUS);
 }

which I would expect to short-circuit when ENABLE is zero.

Thanks,
drew

> 
> Signed-off-by: Nadav Amit <nadav.amit@gmail.com>
> ---
>  arm/timer.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arm/timer.c b/arm/timer.c
> index 2cb8051..0b976a7 100644
> --- a/arm/timer.c
> +++ b/arm/timer.c
> @@ -256,7 +256,7 @@ static void test_timer_pending(struct timer_info *info)
>  	set_timer_irq_enabled(info, true);
>  
>  	report(!info->irq_received, "no interrupt when timer is disabled");
> -	report(!timer_pending(info) && gic_timer_check_state(info, GIC_IRQ_STATE_INACTIVE),
> +	report(gic_timer_check_state(info, GIC_IRQ_STATE_INACTIVE),
>  			"interrupt signal no longer pending");
>  
>  	info->write_cval(now - 1);
> -- 
> 2.34.1
> 

Re: [kvm-unit-tests PATCH] arm64: timer: ignore ISTATUS with disabled timer

[Nadav Amit]

> On Jun 28, 2023, at 4:40 AM, Andrew Jones <andrew.jones@linux.dev> wrote:
> 
> On Wed, Jun 14, 2023 at 05:38:32PM -0700, Nadav Amit wrote:
>> From: Nadav Amit <nadav.amit@gmail.com>
>> 
>> According to ARM specifications for the vtimer (CNTV_CTL_EL0): "When the
>> value of the ENABLE bit is 0, the ISTATUS field is UNKNOWN."
>> 
>> Currently the test, however, does check that ISTATUS is cleared when the
>> ENABLE bit is zero. Remove this check as the value is unknown.
> 
> Hmm, does it? timer_pending() is
> 
> /* Check that the timer condition is met. */
> static bool timer_pending(struct timer_info *info)
> {
>        return (info->read_ctl() & ARCH_TIMER_CTL_ENABLE) &&
>                (info->read_ctl() & ARCH_TIMER_CTL_ISTATUS);
> }
> 
> which I would expect to short-circuit when ENABLE is zero.

Yes, my bad. Drop it.

I had a different unrelated interrupt before that turned on the ENABLE.
This just mask an additional failure.

Obviously we can reset the ENABLE before each test, but I presume it is
fair to say that in general if a certain test fails, subsequent failures
are prone to be false-positives.