Re: [kvm-unit-tests PATCH v2 10/12] arm64: gic: its-trigger: Don't trigger the LPI while it is pending

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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");
>>




[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux