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