Hi Zenghui,
On 3/12/20 10:19 AM, Zenghui Yu wrote:
> 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?
Yes it may be the culprit. Anyway I removed the collection unmap in v6.
By the way are you OK now with v6? I think Drew plans to send a pull
request by the end of this week.
Thanks
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;
>
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
