Re: [kvm-unit-tests PATCH v1 1/4] arm64: split its-trigger test into KVM and TCG variants

Alex Bennée
2021-04-28T16:32:53Z

Alex Bennée <alex.bennee@linaro.org> writes:

> Marc Zyngier <maz@kernel.org> writes:
>
>> On Wed, 28 Apr 2021 15:00:15 +0100,
>> Alexandru Elisei <alexandru.elisei@arm.com> wrote:
>>>
>>> I interpret that as that an INVALL guarantees that a change is
>>> visible, but it the change can become visible even without the
>>> INVALL.
>>
>> Yes. Expecting the LPI to be delivered or not in the absence of an
>> invalidate when its configuration has been altered is wrong. The
>> architecture doesn't guarantee anything of the sort.
>
> Is the underlying hypervisor allowed to invalidate and reload the
> configuration whenever it wants or should it only be driven by the
> guests requests?
>
> I did consider a more nuanced variant of the test that allowed for a
> delivery pre-inval and a pass for post-inval as long as it had been
> delivered one way or another:
>
> --8<---------------cut here---------------start------------->8---
> modified arm/gic.c
> @@ -36,6 +36,7 @@ static struct gic *gic;
> static int acked[NR_CPUS], spurious[NR_CPUS];
> static int irq_sender[NR_CPUS], irq_number[NR_CPUS];
> static cpumask_t ready;
> +static bool under_tcg;
>
> static void nr_cpu_check(int nr)
> {
> @@ -687,6 +688,7 @@ static void test_its_trigger(void)
> struct its_collection *col3;
> struct its_device *dev2, *dev7;
> cpumask_t mask;
> + bool before, after;
>
> if (its_setup1())
> return;
> @@ -734,15 +736,17 @@ static void test_its_trigger(void)
> /*
> * 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
> + * The LPI should not hit. This does however depend on
> + * implementation defined behaviour - under QEMU TCG emulation
> + * it can quite correctly process the event directly.
> */
> gicv3_lpi_set_config(8195, LPI_PROP_DEFAULT);
> stats_reset();
> cpumask_clear(&mask);
> its_send_int(dev2, 20);
> wait_for_interrupts(&mask);
> - report(check_acked(&mask, -1, -1),
> - "dev2/eventid=20 still does not trigger any LPI");
> + before = check_acked(&mask, -1, -1);
> + report_xfail(under_tcg, before, "dev2/eventid=20 still may not trigger any LPI");
>
> /* Now call the invall and check the LPI hits */
> stats_reset();
> @@ -750,8 +754,8 @@ static void test_its_trigger(void)
> cpumask_set_cpu(3, &mask);
> its_send_invall(col3);
> wait_for_interrupts(&mask);
> - report(check_acked(&mask, 0, 8195),
> - "dev2/eventid=20 pending LPI is received");
> + after = check_acked(&mask, 0, 8195);
> + report(before != after, "dev2/eventid=20 pending LPI is
> received");

Actually that should be:

report(after || !before, "dev2/eventid=20 pending LPI is received");

so either the IRQ arrives after the flush or it had previously.

--
Alex Bennée