Re: [PATCH] ArmPkg/ArmGenericTimerVirtCounterLib: deal with broken generic timers

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 20 January 2017 at 14:20, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> Users of ArmGenericTimerVirtCounterLib may execute under virtualization,
> which implies that they may be affected by core errata of the host.
>
> Some implementations of the ARM Generic Timer are affected by errata where
> reads of the counter and reads or writes to the timer value may execute
> incorrectly when issued around the time the counter is incremented by
> the hardware.
>
> Since we can easily work around this without affecting performance too
> much, implement an unconditional workaround that compares two subsequent
> reads of the counter to ensure the value is correct. Note that the number
> for attempts should be limited to avoid breaking platforms such as QEMU
> with TCG emulation, since that has been observed never to return the same
> value from back to back reads of the counter register.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>
> Note that this patch applies on top of the patch 'ArmPkg/ArmLib: remove
> indirection layer from timer register accessors' that I send out earlier
> today.
>
>  ArmPkg/Library/ArmGenericTimerVirtCounterLib/ArmGenericTimerVirtCounterLib.c | 51 ++++++++++++++++++--
>  1 file changed, 48 insertions(+), 3 deletions(-)
>
> diff --git a/ArmPkg/Library/ArmGenericTimerVirtCounterLib/ArmGenericTimerVirtCounterLib.c b/ArmPkg/Library/ArmGenericTimerVirtCounterLib/ArmGenericTimerVirtCounterLib.c
> index 69a4ceb62db6..9fe673e8222c 100644
> --- a/ArmPkg/Library/ArmGenericTimerVirtCounterLib/ArmGenericTimerVirtCounterLib.c
> +++ b/ArmPkg/Library/ArmGenericTimerVirtCounterLib/ArmGenericTimerVirtCounterLib.c
> @@ -70,13 +70,36 @@ ArmGenericTimerGetTimerFreq (
>    return ArmReadCntFrq ();
>  }
>
> +//
> +// The virtual counter may be used under virtualization on a host that
> +// is affected by one of the various errata where reads to the counter
> +// register may return incorrect values when the access occurs at the exact
> +// time that the counter is incremented by the hardware. This affects the
> +// timer as well as the counter.
> +// So repeat  the read until we get the same value twice. Unfortunately,
> +// platforms such as QEMU with TCG emulation (i.e., non-virtualized) appear
> +// never to return the same value twice, so we need to set a retry limit.
> +//
> +#define MAX_RETRIES   200
> +
>  UINTN
>  EFIAPI
>  ArmGenericTimerGetTimerVal (
>    VOID
>    )
>  {
> -  return ArmReadCntvTval ();
> +  UINTN Result;
> +  UINTN Tries;
> +
> +  Tries = 0;
> +  do {
> +    //
> +    // Keep reading until we see the same value twice in a row. See above.
> +    //
> +    Result = ArmReadCntvTval ();
> +  } while (Result != ArmReadCntvTval () && ++Tries < MAX_RETRIES);
> +
> +  return Result;
>  }
>
>
> @@ -86,7 +109,18 @@ ArmGenericTimerSetTimerVal (
>    IN   UINTN   Value
>    )
>  {
> -  ArmWriteCntvTval (Value);
> +  UINTN CounterVal;
> +  UINTN Tries;
> +
> +  Tries = 0;
> +  do {
> +    //
> +    // Read the counter before and after the write to TVAL, to ensure that
> +    // the write to TVAL did not involve a corrupted sample of the counter.
> +    //
> +    CounterVal = ArmReadCntvCt ();
> +    ArmWriteCntvTval (Value);

I wonder if we need an isb here

> +  } while (CounterVal != ArmReadCntvCt () && ++Tries < MAX_RETRIES);
>  }
>
>  UINT64
> @@ -95,7 +129,18 @@ ArmGenericTimerGetSystemCount (
>    VOID
>    )
>  {
> -  return ArmReadCntvCt ();
> +  UINT64 Result;
> +  UINTN Tries;
> +
> +  Tries = 0;
> +  do {
> +    //
> +    // Keep reading until we see the same value twice in a row. See above.
> +    //
> +    Result = ArmReadCntvCt ();
> +  } while (Result != ArmReadCntvCt () && ++Tries < MAX_RETRIES);
> +
> +  return Result;
>  }
>
>  UINTN
> --
> 2.7.4
>