Re: [kvm-unit-tests PATCH v5 10/13] arm/arm64: ITS: INT functional tests

From: Auger Eric <eric.auger@redhat.com>
To: Zenghui Yu <yuzenghui@huawei.com>,
	eric.auger.pro@gmail.com, maz@kernel.org,
	kvmarm@lists.cs.columbia.edu, kvm@vger.kernel.org,
	qemu-devel@nongnu.org, qemu-arm@nongnu.org
Cc: drjones@redhat.com, andre.przywara@arm.com,
	peter.maydell@linaro.org, alexandru.elisei@arm.com,
	thuth@redhat.com
Subject: Re: [kvm-unit-tests PATCH v5 10/13] arm/arm64: ITS: INT functional tests
Date: Wed, 11 Mar 2020 14:48:11 +0100	[thread overview]
Message-ID: <46f0ed1d-3bda-f91b-e2b0-addf1c61c373@redhat.com> (raw)
In-Reply-To: <d3f651a0-2344-4d6e-111b-be133db7e068@huawei.com>

Hi Zenghui,

On 3/11/20 12:59 PM, Zenghui Yu wrote:
> Hi Eric,
> 
> On 2020/3/10 22:54, Eric Auger wrote:
>> Triggers LPIs through the INT command.
>>
>> the test checks the LPI hits the right CPU and triggers
>> the right LPI intid, ie. the translation is correct.
>>
>> Updates to the config table also are tested, along with inv
>> and invall commands.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>
>> ---
> 
> [...]
> 
>> +static void test_its_trigger(void)
>> +{
>> +    struct its_collection *col3, *col2;
>> +    struct its_device *dev2, *dev7;
>> +
>> +    if (its_prerequisites(4))
>> +        return;
>> +
>> +    dev2 = its_create_device(2 /* dev id */, 8 /* nb_ites */);
>> +    dev7 = its_create_device(7 /* dev id */, 8 /* nb_ites */);
>> +
>> +    col3 = its_create_collection(3 /* col id */, 3/* target PE */);
>> +    col2 = its_create_collection(2 /* col id */, 2/* target PE */);
>> +
>> +    gicv3_lpi_set_config(8195, LPI_PROP_DEFAULT);
>> +    gicv3_lpi_set_config(8196, LPI_PROP_DEFAULT);
>> +
>> +    its_send_invall(col2);
>> +    its_send_invall(col3);
> 
> These two INVALLs should be issued after col2 and col3 are mapped,
> otherwise this will cause the INVALL command error as per the spec
> (though KVM doesn't complain it at all).
Yes you're right. reading the spec again:

A command error occurs if any of the following apply:
../..
The collection specified by ICID has not been mapped to an RDbase using
MAPC.

But as mentionned in the cover letter, no real means to retrieve the
error at the moment.

> 
>> +
>> +    report_prefix_push("int");
>> +    /*
>> +     * dev=2, eventid=20  -> lpi= 8195, col=3
>> +     * dev=7, eventid=255 -> lpi= 8196, col=2
>> +     * Trigger dev2, eventid=20 and dev7, eventid=255
>> +     * Check both LPIs hit
>> +     */
>> +
>> +    its_send_mapd(dev2, true);
>> +    its_send_mapd(dev7, true);
>> +
>> +    its_send_mapc(col3, true);
>> +    its_send_mapc(col2, true);
>> +
>> +    its_send_mapti(dev2, 8195 /* lpi id */, 20 /* event id */, col3);
>> +    its_send_mapti(dev7, 8196 /* lpi id */, 255 /* event id */, col2);
>> +
>> +    lpi_stats_expect(3, 8195);
>> +    its_send_int(dev2, 20);
>> +    check_lpi_stats("dev=2, eventid=20  -> lpi= 8195, col=3");
>> +
>> +    lpi_stats_expect(2, 8196);
>> +    its_send_int(dev7, 255);
>> +    check_lpi_stats("dev=7, eventid=255 -> lpi= 8196, col=2");
>> +
>> +    report_prefix_pop();
>> +
>> +    report_prefix_push("inv/invall");
>> +
>> +    /*
>> +     * disable 8195, check dev2/eventid=20 does not trigger the
>> +     * corresponding LPI
>> +     */
>> +    gicv3_lpi_set_config(8195, LPI_PROP_DEFAULT & ~LPI_PROP_ENABLED);
>> +    its_send_inv(dev2, 20);
>> +
>> +    lpi_stats_expect(-1, -1);
>> +    its_send_int(dev2, 20);
>> +    check_lpi_stats("dev2/eventid=20 does not trigger any LPI");
>> +
>> +    /*
>> +     * re-enable the LPI but willingly do not call invall
>> +     * so the change in config is not taken into account.
>> +     * The LPI should not hit
>> +     */
>> +    gicv3_lpi_set_config(8195, LPI_PROP_DEFAULT);
>> +    lpi_stats_expect(-1, -1);
>> +    its_send_int(dev2, 20);
>> +    check_lpi_stats("dev2/eventid=20 still does not trigger any LPI");
>> +
>> +    /* Now call the invall and check the LPI hits */
>> +    its_send_invall(col3);
>> +    lpi_stats_expect(3, 8195);
>> +    its_send_int(dev2, 20);
>> +    check_lpi_stats("dev2/eventid=20 now triggers an LPI");
>> +
>> +    report_prefix_pop();
>> +
>> +    report_prefix_push("mapd valid=false");
>> +    /*
>> +     * Unmap device 2 and check the eventid 20 formerly
>> +     * attached to it does not hit anymore
>> +     */
>> +
>> +    its_send_mapd(dev2, false);
>> +    lpi_stats_expect(-1, -1);
>> +    its_send_int(dev2, 20);
>> +    check_lpi_stats("no LPI after device unmap");
>> +    report_prefix_pop();
>> +
>> +    /* Unmap the collection this time and check no LPI does hit */
>> +    report_prefix_push("mapc valid=false");
>> +    its_send_mapc(col2, false);
> 
> And as for the MAPC, the spec says:
> 
> " When V is 0:
> Behavior is unpredictable if there are interrupts that are mapped to the
> specified collection, with the restriction that further translation
> requests from that device are ignored. "
> 
> So this collection-unmap test may not make sense?
makes sense. Removing it.
> 
>> +    lpi_stats_expect(-1, -1);
>> +    its_send_int(dev7, 255);
>> +    check_lpi_stats("no LPI after collection unmap");
>> +    report_prefix_pop();
>> +}
> 
> [...]
> 
> Otherwise looks good.
Thanks!

Eric
> 
> 
> Thanks,
> Zenghui
> 