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

From: Zenghui Yu <yuzenghui@huawei.com>
To: Marc Zyngier <maz@kernel.org>, Auger Eric <eric.auger@redhat.com>
Cc: <eric.auger.pro@gmail.com>, <kvmarm@lists.cs.columbia.edu>,
	<kvm@vger.kernel.org>, <qemu-devel@nongnu.org>,
	<qemu-arm@nongnu.org>, <drjones@redhat.com>,
	<andre.przywara@arm.com>, <peter.maydell@linaro.org>,
	<alexandru.elisei@arm.com>, <thuth@redhat.com>
Subject: Re: [kvm-unit-tests PATCH v5 10/13] arm/arm64: ITS: INT functional tests
Date: Thu, 12 Mar 2020 17:19:51 +0800	[thread overview]
Message-ID: <7fb9f81f-6520-526d-7031-d3d08cb1dd6a@huawei.com> (raw)
In-Reply-To: <301a8b402ff7e480e927b0f8f8b093f2@kernel.org>

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;

