Re: [kvm-unit-tests PATCH 04/17] arm: gic: Support no IRQs test case

[Alexandru Elisei <alexandru.elisei@arm.com>]

Hi,

On 11/8/19 2:42 PM, Andre Przywara wrote:
> For some tests it would be important to check that an IRQ was *not*
> triggered, for instance to test certain masking operations.
>
> Extend the check_added() function to recognise an empty cpumask to
> detect this situation. The timeout duration is reduced, and the "no IRQs

Why is the timeout duration reduced?

> triggered" case is actually reported as a success in this case.
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  arm/gic.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/arm/gic.c b/arm/gic.c
> index a114009..eca9188 100644
> --- a/arm/gic.c
> +++ b/arm/gic.c
> @@ -66,9 +66,10 @@ static void check_acked(const char *testname, cpumask_t *mask)
>  	int missing = 0, extra = 0, unexpected = 0;
>  	int nr_pass, cpu, i;
>  	bool bad = false;
> +	bool noirqs = cpumask_empty(mask);
>  
>  	/* Wait up to 5s for all interrupts to be delivered */

This comment needs updating.

> -	for (i = 0; i < 50; ++i) {
> +	for (i = 0; i < (noirqs ? 15 : 50); ++i) {
>  		mdelay(100);
>  		nr_pass = 0;
>  		for_each_present_cpu(cpu) {
> @@ -88,7 +89,7 @@ static void check_acked(const char *testname, cpumask_t *mask)
>  				bad = true;
>  			}
>  		}
> -		if (nr_pass == nr_cpus) {
> +		if (!noirqs && nr_pass == nr_cpus) {

This condition is pretty hard to read - what you are doing here is making sure
that when check_acked tests that no irqs have been received, you do the entire for
loop and wait the entire timeout duration. Did I get that right?

How about this (compile tested only):

+               if (noirqs)
+                       /* Wait for the entire timeout duration. */
+                       continue;
+
                if (nr_pass == nr_cpus) {
                        report("%s", !bad, testname);
                        if (i)

>  			report("%s", !bad, testname);
>  			if (i)
>  				report_info("took more than %d ms", i * 100);
> @@ -96,6 +97,11 @@ static void check_acked(const char *testname, cpumask_t *mask)
>  		}
>  	}
>  
> +	if (noirqs && nr_pass == nr_cpus) {
> +		report("%s", !bad, testname);

bad is true only when bad_sender[cpu] != -1 or bad_irq[cpu] != -1, which only get
set in the irq or ipi handlesr, meaning when you do get an interrupt. If nr_pass
== nr_cpus and noirqs, then you shouldn't have gotten an interrupt. I think it's
safe to write it as report("%s", true, testname). I think a short comment above
explaining why we do this check (timeout expired and we haven't gotten any
interrupts) would also improve readability of the code, but that's up to you.

Thanks,
Alex
> +		return;
> +	}
> +
>  	for_each_present_cpu(cpu) {
>  		if (cpumask_test_cpu(cpu, mask)) {
>  			if (!acked[cpu])
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm