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

[Zenghui Yu]

On 2020/3/11 22:00, Marc Zyngier wrote:
> That is still a problem with the ITS. There is no architectural way
> to report an error, even if the error numbers are architected...
>
> One thing we could do though is to implement the stall model (as described
> in 5.3.2). It still doesn't give us the error, but at least the command
> queue would stop on detecting an error.

It would be interesting to see the buggy guest's behavior under the
stall mode. I've used the following diff (absolutely *not* a formal
patch, don't handle CREADR.Stalled and CWRITER.Retry at all) to have
a try, and caught another command error in the 'its-trigger' test.

logs/its-trigger.log:
" INT dev_id=2 event_id=20
lib/arm64/gic-v3-its-cmd.c:194: assert failed: false: INT timeout! "

dmesg:
[13297.711958] ------------[ cut here ]------------
[13297.711964] ITS command error encoding 0x10307

It's the last INT test in test_its_trigger() who has triggered this
error, Eric?


Thanks.

---8<---
diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
index 9d53f545a3d5..5717f5da0f22 100644
--- a/include/kvm/arm_vgic.h
+++ b/include/kvm/arm_vgic.h
@@ -179,6 +179,7 @@ struct vgic_its {
        u64                     cbaser;
        u32                     creadr;
        u32                     cwriter;
+       bool                    stalled;

        /* migration ABI revision in use */
        u32                     abi_rev;
diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index d53d34a33e35..72422b75e627 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -1519,6 +1519,9 @@ static void vgic_its_process_commands(struct kvm 
*kvm, struct vgic_its *its)
        if (!its->enabled)
                return;

+       if (unlikely(its->stalled))
+               return;
+
        cbaser = GITS_CBASER_ADDRESS(its->cbaser);

        while (its->cwriter != its->creadr) {
@@ -1531,9 +1534,34 @@ static void vgic_its_process_commands(struct kvm 
*kvm, struct vgic_its *its)
                 * According to section 6.3.2 in the GICv3 spec we can just
                 * ignore that command then.
                 */
-               if (!ret)
-                       vgic_its_handle_command(kvm, its, cmd_buf);
+               if (ret)
+                       goto done;
+
+               ret = vgic_its_handle_command(kvm, its, cmd_buf);
+
+               /*
+                * Choose the stall mode on detection of command errors.
+                * Guest still can't get the architected error numbers though...
+                */
+               if (ret) {
+                       /* GITS_CREADR.Stalled is set to 1. */
+                       its->stalled = true;
+
+                       /*
+                        * GITS_TYPER.SEIS is 0 atm, no System error will be
+                        * generated.  Instead report error encodings at ITS
+                        * level.
+                        */
+                       WARN_RATELIMIT(ret, "ITS command error encoding 0x%x", 
ret);
+
+                       /*
+                        * GITS_CREADR is not incremented and continues to
+                        * point to the entry that triggered the error.
+                        */
+                       break;
+               }

+done:
                its->creadr += ITS_CMD_SIZE;
                if (its->creadr == ITS_CMD_BUFFER_SIZE(its->cbaser))
                        its->creadr = 0;