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

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

[Ard Biesheuvel]

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);
+  } 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

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

[Ard Biesheuvel]

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
>

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

[Mark Rutland]

[Adding Marc Zyngier]

On Fri, Jan 20, 2017 at 02:20:43PM +0000, Ard Biesheuvel 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.

So far, there are at least two slightly different errata I've seen in
this area: Freescale erratum A-008585, and Hisilicon erratum #161010101.

The first has a workaround in upstream Linux, whereas the latter
apparently requires a different workaround, and is currently under
review.

There may not be a one-size-fits-all solution, here. To that end, I
would strongly suggest that we document precisely which errata we are
trying to handle here.

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

Even on real HW it's possible for back-to-back counter reads to not
(ever) see the same value. That may depend on the relative frequency of
the CPU and counter clocks, e.g. consider FPGAs, or the read of the
counter might always take at least one counter cycle.

Thanks,
Mark.

> 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);
> +  } 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
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

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

[Ard Biesheuvel]

On 20 January 2017 at 15:06, Marc Zyngier <marc.zyngier@arm.com> wrote:
> On 20/01/17 14:41, Mark Rutland wrote:
>> [Adding Marc Zyngier]
>>
>> On Fri, Jan 20, 2017 at 02:20:43PM +0000, Ard Biesheuvel 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.
>>
>> So far, there are at least two slightly different errata I've seen in
>> this area: Freescale erratum A-008585, and Hisilicon erratum #161010101.
>>
>> The first has a workaround in upstream Linux, whereas the latter
>> apparently requires a different workaround, and is currently under
>> review.
>>
>> There may not be a one-size-fits-all solution, here. To that end, I
>> would strongly suggest that we document precisely which errata we are
>> trying to handle here.
>
> Also, is there any way that DT/ACPI could be used to identify the actual
> erratum? When running a guest on my FSL box, my guest launch script adds
> the required property to the guest DT for the kernel to pick it up. Not
> as nice as a "one size fits all" method, but as Mark pointed out, such a
> method may simply not exist...
>

DT should be feasible, given that the UEFI firmware for QEMU does
consume the DT supplied to it. ACPI tables are produced by the
firmware rather than consumed by it, so fortunately we don't need to
care about those.

So if a binding should be defined that describes this erratum, we
could in fact use it to enable it rather than enable it
unconditionally. Are we talking about just 'fsl,erratum-a008585' here?
Or have additional ones been specified already? And is anyone aware if
any work has been done on the QEMU side of this?