Re: [kvm-unit-tests PATCH v3 11/11] arm64: gic: Use IPI test checking for the LPI tests

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

Hi Eric,

On 2/5/21 1:30 PM, Auger Eric wrote:
> Hi Alexandru,
>
> On 1/29/21 5:36 PM, Alexandru Elisei wrote:
>> The LPI code validates a result similarly to the IPI tests, by checking if
>> the target CPU received the interrupt with the expected interrupt number.
>> However, the LPI tests invent their own way of checking the test results by
>> creating a global struct (lpi_stats), using a separate interrupt handler
>> (lpi_handler) and test function (check_lpi_stats).
>>
>> There are several areas that can be improved in the LPI code, which are
>> already covered by the IPI tests:
>>
>> - check_lpi_stats() doesn't take into account that the target CPU can
>>   receive the correct interrupt multiple times.
>> - check_lpi_stats() doesn't take into the account the scenarios where all
>>   online CPUs can receive the interrupt, but the target CPU is the last CPU
>>   that touches lpi_stats.observed.
>> - Insufficient or missing memory synchronization.
>>
>> Instead of duplicating code, let's convert the LPI tests to use
>> check_acked() and the same interrupt handler as the IPI tests, which has
>> been renamed to irq_handler() to avoid any confusion.
>>
>> check_lpi_stats() has been replaced with check_acked() which, together with
>> using irq_handler(), instantly gives us more correctness checks and proper
>> memory synchronization between threads. lpi_stats.expected has been
>> replaced by the CPU mask and the expected interrupt number arguments to
>> check_acked(), with no change in semantics.
>>
>> lpi_handler() aborted the test if the interrupt number was not an LPI. This
>> was changed in favor of allowing the test to continue, as it will fail in
>> check_acked(), but possibly print information useful for debugging. If the
>> test receives spurious interrupts, those are reported via report_info() at
>> the end of the test for consistency with the IPI tests, which don't treat
>> spurious interrupts as critical errors.
>>
>> In the spirit of code reuse, secondary_lpi_tests() has been replaced with
>> ipi_recv() because the two are now identical; ipi_recv() has been renamed
>> to irq_recv(), similarly to irq_handler(), to avoid confusion.
>>
>> CC: Eric Auger <eric.auger@redhat.com>
>> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
>> ---
>>  arm/gic.c | 190 +++++++++++++++++++++++++-----------------------------
>>  1 file changed, 87 insertions(+), 103 deletions(-)
>>
>> [..]
>> @@ -796,18 +737,31 @@ static void test_its_trigger(void)
>>  	 * The LPI should not hit
>>  	 */
>>  	gicv3_lpi_set_config(8195, LPI_PROP_DEFAULT);
>> -	lpi_stats_expect(-1, -1);
>> +	stats_reset();
>> +	cpumask_clear(&mask);
>>  	its_send_int(dev2, 20);
>> -	check_lpi_stats("dev2/eventid=20 still does not trigger any LPI");
>> +	wait_for_interrupts(&mask);
>> +	report(check_acked(&mask, -1, -1),
>> +			"dev2/eventid=20 still does not trigger any LPI");
>>  
>>  	/* Now call the invall and check the LPI hits */
>> +	stats_reset();
>> +	/* The barrier is from its_send_int() */
>> +	wmb();
> In v1 it was envisionned to add the wmb in __its_send_it but I fail to
> see it. Is it implicit in some way?

Thank you for having a look at this, it seems I forgot to remove this barrier.

The barriers in __its_send_int() and the one above are not needed because the
barrier is already present in its_send_invall() -> its_send_single_command() ->
its_post_commands() -> writeq() (the removal from __its_send_int() is also
explained in the cover letter).

I'll remove the wmb() barrier in the next version.

Thanks,

Alex

>
> Thanks
>
> Eric
>> +	cpumask_clear(&mask);
>> +	cpumask_set_cpu(3, &mask);
>>  	its_send_invall(col3);
>> -	lpi_stats_expect(3, 8195);
>> -	check_lpi_stats("dev2/eventid=20 pending LPI is received");
>> +	wait_for_interrupts(&mask);
>> +	report(check_acked(&mask, 0, 8195),
>> +			"dev2/eventid=20 pending LPI is received");
>>  
>> -	lpi_stats_expect(3, 8195);
>> +	stats_reset();
>> +	cpumask_clear(&mask);
>> +	cpumask_set_cpu(3, &mask);
>>  	its_send_int(dev2, 20);
>> -	check_lpi_stats("dev2/eventid=20 now triggers an LPI");
>> +	wait_for_interrupts(&mask);
>> +	report(check_acked(&mask, 0, 8195),
>> +			"dev2/eventid=20 now triggers an LPI");
>>  
>>  	report_prefix_pop();
>>  
>> @@ -818,9 +772,13 @@ static void test_its_trigger(void)
>>  	 */
>>  
>>  	its_send_mapd(dev2, false);
>> -	lpi_stats_expect(-1, -1);
>> +	stats_reset();
>> +	cpumask_clear(&mask);
>>  	its_send_int(dev2, 20);
>> -	check_lpi_stats("no LPI after device unmap");
>> +	wait_for_interrupts(&mask);
>> +	report(check_acked(&mask, -1, -1), "no LPI after device unmap");
>> +
>> +	check_spurious();
>>  	report_prefix_pop();
>>  }
>>  
>> @@ -828,6 +786,7 @@ static void test_its_migration(void)
>>  {
>>  	struct its_device *dev2, *dev7;
>>  	bool test_skipped = false;
>> +	cpumask_t mask;
>>  
>>  	if (its_setup1()) {
>>  		test_skipped = true;
>> @@ -844,13 +803,23 @@ do_migrate:
>>  	if (test_skipped)
>>  		return;
>>  
>> -	lpi_stats_expect(3, 8195);
>> +	stats_reset();
>> +	cpumask_clear(&mask);
>> +	cpumask_set_cpu(3, &mask);
>>  	its_send_int(dev2, 20);
>> -	check_lpi_stats("dev2/eventid=20 triggers LPI 8195 on PE #3 after migration");
>> +	wait_for_interrupts(&mask);
>> +	report(check_acked(&mask, 0, 8195),
>> +			"dev2/eventid=20 triggers LPI 8195 on PE #3 after migration");
>>  
>> -	lpi_stats_expect(2, 8196);
>> +	stats_reset();
>> +	cpumask_clear(&mask);
>> +	cpumask_set_cpu(2, &mask);
>>  	its_send_int(dev7, 255);
>> -	check_lpi_stats("dev7/eventid=255 triggers LPI 8196 on PE #2 after migration");
>> +	wait_for_interrupts(&mask);
>> +	report(check_acked(&mask, 0, 8196),
>> +			"dev7/eventid=255 triggers LPI 8196 on PE #2 after migration");
>> +
>> +	check_spurious();
>>  }
>>  
>>  #define ERRATA_UNMAPPED_COLLECTIONS "ERRATA_8c58be34494b"
>> @@ -860,6 +829,7 @@ static void test_migrate_unmapped_collection(void)
>>  	struct its_collection *col = NULL;
>>  	struct its_device *dev2 = NULL, *dev7 = NULL;
>>  	bool test_skipped = false;
>> +	cpumask_t mask;
>>  	int pe0 = 0;
>>  	u8 config;
>>  
>> @@ -894,17 +864,27 @@ do_migrate:
>>  	its_send_mapc(col, true);
>>  	its_send_invall(col);
>>  
>> -	lpi_stats_expect(2, 8196);
>> +	stats_reset();
>> +	cpumask_clear(&mask);
>> +	cpumask_set_cpu(2, &mask);
>>  	its_send_int(dev7, 255);
>> -	check_lpi_stats("dev7/eventid= 255 triggered LPI 8196 on PE #2");
>> +	wait_for_interrupts(&mask);
>> +	report(check_acked(&mask, 0, 8196),
>> +			"dev7/eventid= 255 triggered LPI 8196 on PE #2");
>>  
>>  	config = gicv3_lpi_get_config(8192);
>>  	report(config == LPI_PROP_DEFAULT,
>>  	       "Config of LPI 8192 was properly migrated");
>>  
>> -	lpi_stats_expect(pe0, 8192);
>> +	stats_reset();
>> +	cpumask_clear(&mask);
>> +	cpumask_set_cpu(pe0, &mask);
>>  	its_send_int(dev2, 0);
>> -	check_lpi_stats("dev2/eventid = 0 triggered LPI 8192 on PE0");
>> +	wait_for_interrupts(&mask);
>> +	report(check_acked(&mask, 0, 8192),
>> +			"dev2/eventid = 0 triggered LPI 8192 on PE0");
>> +
>> +	check_spurious();
>>  }
>>  
>>  static void test_its_pending_migration(void)
>> @@ -961,6 +941,10 @@ static void test_its_pending_migration(void)
>>  	pendbaser = readq(ptr);
>>  	writeq(pendbaser & ~GICR_PENDBASER_PTZ, ptr);
>>  
>> +	/*
>> +	 * Reset and initialization values for acked are the same, so we don't
>> +	 * need to explicitely call stats_reset().
>> +	 */
>>  	gicv3_lpi_rdist_enable(pe0);
>>  	gicv3_lpi_rdist_enable(pe1);
>>  
>>