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@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.
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@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");
>>
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
