[kvm-unit-tests PATCH v1 0/4] enable LPI and ITS for TCG

[kvm-unit-tests PATCH v1 0/4] enable LPI and ITS for TCG

[Alex Bennée]

This is a companion to Shashi's series enabling LPI and ITS features
for QEMU's TCG emulation. This is part of our push for a sbsa-ref
platform which needs a more modern set of features.

  From: Shashi Mallela <shashi.mallela@linaro.org>
  Subject: [PATCH v2 0/8] GICv3 LPI and ITS feature implementation
  Date: Wed, 31 Mar 2021 22:41:44 -0400
  Message-Id: <20210401024152.203896-1-shashi.mallela@linaro.org>

Most of the changes are minor except the its-trigger test which skips
invall handling checks which I think is relying on IMPDEF behaviour
which we can't probe for. There is also a hilarious work around to
some limitations in the run_migration() script in the last patch.

Alex Bennée (4):
  arm64: split its-trigger test into KVM and TCG variants
  scripts/arch-run: don't use deprecated server/nowait options
  arm64: enable its-migration tests for TCG
  arm64: split its-migrate-unmapped-collection into KVM and TCG variants

 scripts/arch-run.bash |  4 +--
 arm/gic.c             | 67 ++++++++++++++++++++++++++-----------------
 arm/unittests.cfg     | 23 ++++++++++++---
 3 files changed, 62 insertions(+), 32 deletions(-)

-- 
2.20.1

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

[Alex Bennée]

A few of the its-trigger tests rely on IMPDEF behaviour where caches
aren't flushed before invall events. However TCG emulation doesn't
model any invall behaviour and as we can't probe for it we need to be
told. Split the test into a KVM and TCG variant and skip the invall
tests when under TCG.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Cc: Shashi Mallela <shashi.mallela@linaro.org>
---
 arm/gic.c         | 60 +++++++++++++++++++++++++++--------------------
 arm/unittests.cfg | 11 ++++++++-
 2 files changed, 45 insertions(+), 26 deletions(-)

diff --git a/arm/gic.c b/arm/gic.c
index 98135ef..96a329d 100644
--- a/arm/gic.c
+++ b/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)
 {
@@ -734,32 +735,38 @@ 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");
-
-	/* Now call the invall and check the LPI hits */
-	stats_reset();
-	cpumask_clear(&mask);
-	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");
-
-	stats_reset();
-	cpumask_clear(&mask);
-	cpumask_set_cpu(3, &mask);
-	its_send_int(dev2, 20);
-	wait_for_interrupts(&mask);
-	report(check_acked(&mask, 0, 8195),
-			"dev2/eventid=20 now triggers an LPI");
+	if (under_tcg) {
+		report_skip("checking LPI triggers without invall");
+	} else {
+		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");
+
+		/* Now call the invall and check the LPI hits */
+		stats_reset();
+		cpumask_clear(&mask);
+		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");
+
+		stats_reset();
+		cpumask_clear(&mask);
+		cpumask_set_cpu(3, &mask);
+		its_send_int(dev2, 20);
+		wait_for_interrupts(&mask);
+		report(check_acked(&mask, 0, 8195),
+		       "dev2/eventid=20 now triggers an LPI");
+	}
 
 	report_prefix_pop();
 
@@ -981,6 +988,9 @@ int main(int argc, char **argv)
 	if (argc < 2)
 		report_abort("no test specified");
 
+	if (argc == 3 && strcmp(argv[2], "tcg") == 0)
+		under_tcg = true;
+
 	if (strcmp(argv[1], "ipi") == 0) {
 		report_prefix_push(argv[1]);
 		nr_cpu_check(2);
diff --git a/arm/unittests.cfg b/arm/unittests.cfg
index f776b66..c72dc34 100644
--- a/arm/unittests.cfg
+++ b/arm/unittests.cfg
@@ -184,13 +184,22 @@ extra_params = -machine gic-version=3 -append 'its-introspection'
 groups = its
 arch = arm64
 
-[its-trigger]
+[its-trigger-kvm]
 file = gic.flat
 smp = $MAX_SMP
+accel = kvm
 extra_params = -machine gic-version=3 -append 'its-trigger'
 groups = its
 arch = arm64
 
+[its-trigger-tcg]
+file = gic.flat
+smp = $MAX_SMP
+accel = tcg
+extra_params = -machine gic-version=3 -append 'its-trigger tcg'
+groups = its
+arch = arm64
+
 [its-migration]
 file = gic.flat
 smp = $MAX_SMP
-- 
2.20.1

[kvm-unit-tests PATCH v1 2/4] scripts/arch-run: don't use deprecated server/nowait options

[Alex Bennée]

The very fact that QEMU drops the deprecation warning while running is
enough to confuse the its-migration test into failing. The boolean
options server and wait have accepted the long form options for a long
time.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 scripts/arch-run.bash | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/scripts/arch-run.bash b/scripts/arch-run.bash
index 5997e38..70693f2 100644
--- a/scripts/arch-run.bash
+++ b/scripts/arch-run.bash
@@ -122,14 +122,14 @@ run_migration ()
 	trap 'kill 0; exit 2' INT TERM
 	trap 'rm -f ${migout1} ${migsock} ${qmp1} ${qmp2} ${fifo}' RETURN EXIT
 
-	eval "$@" -chardev socket,id=mon1,path=${qmp1},server,nowait \
+	eval "$@" -chardev socket,id=mon1,path=${qmp1},server=on,wait=off \
 		-mon chardev=mon1,mode=control | tee ${migout1} &
 
 	# We have to use cat to open the named FIFO, because named FIFO's, unlike
 	# pipes, will block on open() until the other end is also opened, and that
 	# totally breaks QEMU...
 	mkfifo ${fifo}
-	eval "$@" -chardev socket,id=mon2,path=${qmp2},server,nowait \
+	eval "$@" -chardev socket,id=mon2,path=${qmp2},server=on,wait=off \
 		-mon chardev=mon2,mode=control -incoming unix:${migsock} < <(cat ${fifo}) &
 	incoming_pid=`jobs -l %+ | awk '{print$2}'`
 
-- 
2.20.1

[kvm-unit-tests PATCH v1 3/4] arm64: enable its-migration tests for TCG

[Alex Bennée]

With the support for TCG emulated GIC we can also test these now. You
need to call run_tests.sh with -a to trigger the
its-migrate-unmapped-collection test which obviously doesn't need the
KVM errata to run in TCG system emulation mode.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 arm/unittests.cfg | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/arm/unittests.cfg b/arm/unittests.cfg
index c72dc34..d4dbc8b 100644
--- a/arm/unittests.cfg
+++ b/arm/unittests.cfg
@@ -203,7 +203,6 @@ arch = arm64
 [its-migration]
 file = gic.flat
 smp = $MAX_SMP
-accel = kvm
 extra_params = -machine gic-version=3 -append 'its-migration'
 groups = its migration
 arch = arm64
@@ -211,7 +210,6 @@ arch = arm64
 [its-pending-migration]
 file = gic.flat
 smp = $MAX_SMP
-accel = kvm
 extra_params = -machine gic-version=3 -append 'its-pending-migration'
 groups = its migration
 arch = arm64
@@ -219,7 +217,6 @@ arch = arm64
 [its-migrate-unmapped-collection]
 file = gic.flat
 smp = $MAX_SMP
-accel = kvm
 extra_params = -machine gic-version=3 -append 'its-migrate-unmapped-collection'
 groups = its migration
 arch = arm64
-- 
2.20.1

[kvm-unit-tests PATCH v1 4/4] arm64: split its-migrate-unmapped-collection into KVM and TCG variants

[Alex Bennée]

When running the test in TCG we are basically running on bare metal so
don't rely on having a particular kernel errata applied.

You might wonder why we handle this with a totally new test name
instead of adjusting the append as we have before? Well the
run_migration shell script uses eval "$@" which unwraps the -append
leading to any second parameter being split and leaving QEMU very
confused and the test hanging. This seemed simpler than re-writing all
the test running logic in something sane ;-)

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Cc: Shashi Mallela <shashi.mallela@linaro.org>
---
 arm/gic.c         |  7 ++++++-
 arm/unittests.cfg | 11 ++++++++++-
 2 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/arm/gic.c b/arm/gic.c
index 96a329d..3bc7477 100644
--- a/arm/gic.c
+++ b/arm/gic.c
@@ -843,7 +843,7 @@ static void test_migrate_unmapped_collection(void)
 		goto do_migrate;
 	}
 
-	if (!errata(ERRATA_UNMAPPED_COLLECTIONS)) {
+	if (!errata(ERRATA_UNMAPPED_COLLECTIONS) && !under_tcg) {
 		report_skip("Skipping test, as this test hangs without the fix. "
 			    "Set %s=y to enable.", ERRATA_UNMAPPED_COLLECTIONS);
 		test_skipped = true;
@@ -1017,6 +1017,11 @@ int main(int argc, char **argv)
 		report_prefix_push(argv[1]);
 		test_migrate_unmapped_collection();
 		report_prefix_pop();
+	} else if (!strcmp(argv[1], "its-migrate-unmapped-collection-tcg")) {
+		under_tcg = true;
+		report_prefix_push(argv[1]);
+		test_migrate_unmapped_collection();
+		report_prefix_pop();
 	} else if (strcmp(argv[1], "its-introspection") == 0) {
 		report_prefix_push(argv[1]);
 		test_its_introspection();
diff --git a/arm/unittests.cfg b/arm/unittests.cfg
index d4dbc8b..e8f2e74 100644
--- a/arm/unittests.cfg
+++ b/arm/unittests.cfg
@@ -214,13 +214,22 @@ extra_params = -machine gic-version=3 -append 'its-pending-migration'
 groups = its migration
 arch = arm64
 
-[its-migrate-unmapped-collection]
+[its-migrate-unmapped-collection-kvm]
 file = gic.flat
 smp = $MAX_SMP
+accel = kvm
 extra_params = -machine gic-version=3 -append 'its-migrate-unmapped-collection'
 groups = its migration
 arch = arm64
 
+[its-migrate-unmapped-collection-tcg]
+file = gic.flat
+smp = $MAX_SMP
+accel = tcg
+extra_params = -machine gic-version=3 -append 'its-migrate-unmapped-collection-tcg'
+groups = its migration
+arch = arm64
+
 # Test PSCI emulation
 [psci]
 file = psci.flat
-- 
2.20.1

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

[Marc Zyngier]

On 2021-04-28 11:18, Alex Bennée wrote:
> A few of the its-trigger tests rely on IMPDEF behaviour where caches
> aren't flushed before invall events. However TCG emulation doesn't
> model any invall behaviour and as we can't probe for it we need to be
> told. Split the test into a KVM and TCG variant and skip the invall
> tests when under TCG.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Cc: Shashi Mallela <shashi.mallela@linaro.org>
> ---
>  arm/gic.c         | 60 +++++++++++++++++++++++++++--------------------
>  arm/unittests.cfg | 11 ++++++++-
>  2 files changed, 45 insertions(+), 26 deletions(-)
> 
> diff --git a/arm/gic.c b/arm/gic.c
> index 98135ef..96a329d 100644
> --- a/arm/gic.c
> +++ b/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)
>  {
> @@ -734,32 +735,38 @@ 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.

It looks to me that you are using an IMPDEF behaviour of *TCG*
here. The programming model mandates that there is an invalidation
if you change the configuration of the LPI.

         M.
-- 
Jazz is not dead. It just smells funny...

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

[Alex Bennée]

Marc Zyngier <maz@kernel.org> writes:

> On 2021-04-28 11:18, Alex Bennée wrote:
>> A few of the its-trigger tests rely on IMPDEF behaviour where caches
>> aren't flushed before invall events. However TCG emulation doesn't
>> model any invall behaviour and as we can't probe for it we need to be
>> told. Split the test into a KVM and TCG variant and skip the invall
>> tests when under TCG.
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> Cc: Shashi Mallela <shashi.mallela@linaro.org>
>> ---
>>  arm/gic.c         | 60 +++++++++++++++++++++++++++--------------------
>>  arm/unittests.cfg | 11 ++++++++-
>>  2 files changed, 45 insertions(+), 26 deletions(-)
>> diff --git a/arm/gic.c b/arm/gic.c
>> index 98135ef..96a329d 100644
>> --- a/arm/gic.c
>> +++ b/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)
>>  {
>> @@ -734,32 +735,38 @@ 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.
>
> It looks to me that you are using an IMPDEF behaviour of *TCG*
> here. The programming model mandates that there is an invalidation
> if you change the configuration of the LPI.

But does it mandate that the LPI cannot be sent until the invalidation?

>
>         M.


-- 
Alex Bennée

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

[Alexandru Elisei]

Hi,

On 4/28/21 1:06 PM, Alex Bennée wrote:
> Marc Zyngier <maz@kernel.org> writes:
>
>> On 2021-04-28 11:18, Alex Bennée wrote:
>>> A few of the its-trigger tests rely on IMPDEF behaviour where caches
>>> aren't flushed before invall events. However TCG emulation doesn't
>>> model any invall behaviour and as we can't probe for it we need to be
>>> told. Split the test into a KVM and TCG variant and skip the invall
>>> tests when under TCG.
>>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>>> Cc: Shashi Mallela <shashi.mallela@linaro.org>
>>> ---
>>>  arm/gic.c         | 60 +++++++++++++++++++++++++++--------------------
>>>  arm/unittests.cfg | 11 ++++++++-
>>>  2 files changed, 45 insertions(+), 26 deletions(-)
>>> diff --git a/arm/gic.c b/arm/gic.c
>>> index 98135ef..96a329d 100644
>>> --- a/arm/gic.c
>>> +++ b/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)
>>>  {
>>> @@ -734,32 +735,38 @@ 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.
>> It looks to me that you are using an IMPDEF behaviour of *TCG*
>> here. The programming model mandates that there is an invalidation
>> if you change the configuration of the LPI.
> But does it mandate that the LPI cannot be sent until the invalidation?

I think Marc is referring to this section of the GIC architecture (Arm IHI 0069F,
page 5-82, I've highlighted the interesting bits):

"A Redistributor can cache the information from the LPI Configuration tables
pointed to by GICR_PROPBASER, when GICR_CTLR.EnableLPI == 1, subject to all of the
following rules:
* Whether or not one or more caches are present is IMPLEMENTATION DEFINED. Where
at least one cache is present, the structure and size is IMPLEMENTATION DEFINED.
* An LPI Configuration table entry might be allocated into the cache at any time.
* A cached LPI Configuration table entry is not guaranteed to remain in the cache.
* A cached LPI Configuration table entry *is not guaranteed to remain incoherent
with memory*.
* A change to the LPI configuration *is not guaranteed to be visible until an
appropriate invalidation operation has completed*"

I interpret that as that an INVALL guarantees that a change is visible, but it the
change can become visible even without the INVALL.

The test relies on the fact that changes to the LPI tables are not visible *under
KVM* until the INVALL command, but that's not necessarily the case on real
hardware. To match the spec, I think the test "dev2/eventid=20 still does not
trigger any LPI" should be removed and the stats reset should take place before
the configuration for LPI 8195 is set to the default.

Thanks,

Alex

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

[Marc Zyngier]

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.

> The test relies on the fact that changes to the LPI tables are not
> visible *under KVM* until the INVALL command, but that's not
> necessarily the case on real hardware. To match the spec, I think
> the test "dev2/eventid=20 still does not trigger any LPI" should be
> removed and the stats reset should take place before the
> configuration for LPI 8195 is set to the default.

If that's what the test expects (I haven't tried to investigate), it
should be dropped completely, rather than trying to sidestep it for
TCG.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

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

[Auger Eric]

Hi,

On 4/28/21 4:36 PM, Marc Zyngier wrote:
> 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.
> 
>> The test relies on the fact that changes to the LPI tables are not
>> visible *under KVM* until the INVALL command, but that's not
>> necessarily the case on real hardware. To match the spec, I think
>> the test "dev2/eventid=20 still does not trigger any LPI" should be
>> removed and the stats reset should take place before the
>> configuration for LPI 8195 is set to the default.

Yes I do agree with Alexandru and Marc after another reading of the
spec. I initially thought the INVALL was the gate keeper for the new
config but that sounds wrong. This test shall be removed then.

Eric
> 
> If that's what the test expects (I haven't tried to investigate), it
> should be dropped completely, rather than trying to sidestep it for
> TCG.
> 
> Thanks,
> 
> 	M.
> 

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

[Alex Bennée]

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");
 
 	stats_reset();
 	cpumask_clear(&mask);
@@ -759,7 +763,7 @@ static void test_its_trigger(void)
 	its_send_int(dev2, 20);
 	wait_for_interrupts(&mask);
 	report(check_acked(&mask, 0, 8195),
-			"dev2/eventid=20 now triggers an LPI");
+	       "dev2/eventid=20 now triggers an LPI");
 
 	report_prefix_pop();
 
@@ -981,6 +985,9 @@ int main(int argc, char **argv)
 	if (argc < 2)
 		report_abort("no test specified");
 
+	if (argc == 3 && strcmp(argv[2], "tcg") == 0)
+		under_tcg = true;
+
 	if (strcmp(argv[1], "ipi") == 0) {
 		report_prefix_push(argv[1]);
 		nr_cpu_check(2);
--8<---------------cut here---------------end--------------->8---

But that gets confused (that may be something for Sashi to look at):

  ITS: MAPD devid=2 size = 0x8 itt=0x40440000 valid=1
  ITS: MAPD devid=7 size = 0x8 itt=0x40450000 valid=1
  MAPC col_id=3 target_addr = 0x30000 valid=1
  MAPC col_id=2 target_addr = 0x20000 valid=1
  INVALL col_id=2
  INVALL col_id=3
  MAPTI dev_id=2 event_id=20 -> phys_id=8195, col_id=3
  MAPTI dev_id=7 event_id=255 -> phys_id=8196, col_id=2
  INT dev_id=2 event_id=20
  PASS: gicv3: its-trigger: int: dev=2, eventid=20  -> lpi= 8195, col=3
  INT dev_id=7 event_id=255
  PASS: gicv3: its-trigger: int: dev=7, eventid=255 -> lpi= 8196, col=2
  INV dev_id=2 event_id=20
  INT dev_id=2 event_id=20
  PASS: gicv3: its-trigger: inv/invall: dev2/eventid=20 does not trigger any LPI
  INT dev_id=2 event_id=20
  INFO: gicv3: its-trigger: inv/invall: interrupts timed-out (5s)
  INFO: gicv3: its-trigger: inv/invall: cpu3 received wrong irq 8195
  INFO: gicv3: its-trigger: inv/invall: ACKS: missing=0 extra=0 unexpected=1
  XFAIL: gicv3: its-trigger: inv/invall: dev2/eventid=20 still may not trigger any LPI
  INVALL col_id=3
  INFO: gicv3: its-trigger: inv/invall: interrupts timed-out (5s)
  INFO: gicv3: its-trigger: inv/invall: ACKS: missing=1 extra=0 unexpected=0
  FAIL: gicv3: its-trigger: inv/invall: dev2/eventid=20 pending LPI is received
  INT dev_id=2 event_id=20
  PASS: gicv3: its-trigger: inv/invall: dev2/eventid=20 now triggers an LPI
  ITS: MAPD devid=2 size = 0x8 itt=0x40440000 valid=0
  INT dev_id=2 event_id=20
  PASS: gicv3: its-trigger: mapd valid=false: no LPI after device unmap
  SUMMARY: 7 tests, 1 unexpected failures, 1 expected failures

>> The test relies on the fact that changes to the LPI tables are not
>> visible *under KVM* until the INVALL command, but that's not
>> necessarily the case on real hardware. To match the spec, I think
>> the test "dev2/eventid=20 still does not trigger any LPI" should be
>> removed and the stats reset should take place before the
>> configuration for LPI 8195 is set to the default.
>
> If that's what the test expects (I haven't tried to investigate), it
> should be dropped completely, rather than trying to sidestep it for
> TCG.

All three parts of that section?

	report(check_acked(&mask, -1, -1),
			"dev2/eventid=20 still does not trigger any LPI");
	report(check_acked(&mask, 0, 8195),
			"dev2/eventid=20 pending LPI is received");
	report(check_acked(&mask, 0, 8195),
			"dev2/eventid=20 now triggers an LPI");


-- 
Alex Bennée

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

[Alex Bennée]

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

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

[Marc Zyngier]

On Wed, 28 Apr 2021 16:37:45 +0100,
Alex Bennée <alex.bennee@linaro.org> wrote:
> 
> 
> 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?

The HW can do it at any time. It all depends on whether the RD has
cached this LPI configuration or not. KVM relies on the required
invalidation as a hook to reload the cached state, as it has an
infinite LPI configuration cache, while TCG doesn't have a cache at
all. Both approaches are valid implementations.

> 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

This first point is *wrong*. From the architecture spec:

<quote>
* A change to the LPI configuration is not guaranteed to be visible
  until an appropriate invalidation operation has completed:

  - If one or more ITS is implemented, invalidation is performed using
    the INV or INVALL command. A SYNC command completes the INV and
    INVALL commands.
</quote>

*not guaranteed* means that it may fire, it may not.

> +	 * implementation defined behaviour - under QEMU TCG emulation
> +	 * it can quite correctly process the event directly.

I really don't see the point in testing IMPDEF behaviours. We should
test for architectural compliance, not for implementation choices.

	M.

-- 
Without deviation from the norm, progress is not possible.