Hi Andre, On 12/18/20 6:15 PM, André Przywara wrote: > 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, ..."
That's a good point, I'll reword it. > >> 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 <[email protected]> >> Suggested-by: Zenghui Yu <[email protected]> >> Signed-off-by: Alexandru Elisei <[email protected]> > 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. That's a good point, I must admit that I didn't check how caching is defined by the architecture. I would prefer creating a patch independent of this series to change what test_its_trigger() checks for, to get input from Eric just for that particular change. Thanks, Alex > > For this patch, with the message fixed: > > Reviewed-by: Andre Przywara <[email protected]> > > 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"); >> _______________________________________________ kvmarm mailing list [email protected] https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
