| Message ID | 20201217141400.106137-11-alexandru.elisei@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | GIC fixes and improvements | expand |
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@redhat.com> > Suggested-by: Zenghui Yu <yuzenghui@huawei.com> > Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com> 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@arm.com> 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"); >
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 <eric.auger@redhat.com> >> Suggested-by: Zenghui Yu <yuzenghui@huawei.com> >> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com> > 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 <andre.przywara@arm.com> > > 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"); >>
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");
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 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@redhat.com> Suggested-by: Zenghui Yu <yuzenghui@huawei.com> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com> --- arm/gic.c | 3 +++ 1 file changed, 3 insertions(+)