On 17/12/2020 14:13, Alexandru Elisei wrote:
> The its-trigger test checks that LPI 8195 is not delivered to the CPU while
> it is disabled at the ITS level. After that it is re-enabled and the test
> checks that the interrupt is properly asserted. After it's re-enabled and
> before the stats are examined, the test triggers the interrupt again, which
> can lead to the same interrupt being delivered twice: once after the
> configuration invalidation and before the INT command, and once after the
> INT command.
>
> Get rid of the INT command after the interrupt is re-enabled to prevent the
This is confusing to read, since you don't remove anything in the patch.
Can you reword this? Something like "Before explicitly triggering the
interrupt, check that the unmasking worked, ..."
> LPI from being asserted twice and add a separate check to test that the INT
> command still works for the now re-enabled LPI 8195.
>
> CC: Auger Eric <eric.auger@xxxxxxxxxx>
> Suggested-by: Zenghui Yu <yuzenghui@xxxxxxxxxx>
> Signed-off-by: Alexandru Elisei <alexandru.elisei@xxxxxxx>
Otherwise this looks fine, but I think there is another flaw: There is
no requirement that an INV(ALL) is *needed* to update the status, it
could also update anytime (think: "cache invalidate").
The KVM ITS emulation *only* bothers to read the memory on an INV(ALL)
command, so that matches the test. But that's not how unit-tests should
work ;-)
But that's a separate issue, just mentioning this to not forget about it.
For this patch, with the message fixed:
Reviewed-by: Andre Przywara <andre.przywara@xxxxxxx>
Cheers,
Andre
> ---
> arm/gic.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/arm/gic.c b/arm/gic.c
> index fb91861900b7..aa3aa1763984 100644
> --- a/arm/gic.c
> +++ b/arm/gic.c
> @@ -805,6 +805,9 @@ static void test_its_trigger(void)
>
> /* Now call the invall and check the LPI hits */
> its_send_invall(col3);
> + lpi_stats_expect(3, 8195);
> + check_lpi_stats("dev2/eventid=20 pending LPI is received");
> +
> lpi_stats_expect(3, 8195);
> its_send_int(dev2, 20);
> check_lpi_stats("dev2/eventid=20 now triggers an LPI");
>
