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;
