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

["Alex Bennée" <alex.bennee@linaro.org>]

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