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

[Auger Eric]

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;
>