Re: [PATCH 1/1] EmbeddedPkg: Fix multiple Virtual RTC issues

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

On Tue, 26 Feb 2019 at 00:52, Pete Batard <pete@akeo.ie> wrote:
>
> LibGetTime():
> - Two variables were used for the epoch, where only one should have been.
> - Also harmonize variable name to match the one used in LibSetTime.
> LiBSetTime():
> - Address possible underflows if time is set to start of epoch.
> - Ensure that time being read does actually match time that was manually
>   set (plus the time elapsed since), by subtracting number of seconds
>   since reset.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Pete Batard <pete@akeo.ie>

Thanks Pete

Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

Pushed as 9ab4ec518882..1342d7679e10
> ---
>  EmbeddedPkg/Library/VirtualRealTimeClockLib/VirtualRealTimeClockLib.c | 34 ++++++++++++++------
>  1 file changed, 25 insertions(+), 9 deletions(-)
>
> diff --git a/EmbeddedPkg/Library/VirtualRealTimeClockLib/VirtualRealTimeClockLib.c b/EmbeddedPkg/Library/VirtualRealTimeClockLib/VirtualRealTimeClockLib.c
> index 4c354730d02b..5559106b696f 100644
> --- a/EmbeddedPkg/Library/VirtualRealTimeClockLib/VirtualRealTimeClockLib.c
> +++ b/EmbeddedPkg/Library/VirtualRealTimeClockLib/VirtualRealTimeClockLib.c
> @@ -54,13 +54,12 @@ LibGetTime (
>    )
>  {
>    EFI_STATUS  Status;
> -  UINT32      EpochSeconds;
>    INT16       TimeZone;
>    UINT8       Daylight;
>    UINT64      Freq;
>    UINT64      Counter;
>    UINT64      Remainder;
> -  UINTN       ElapsedSeconds;
> +  UINTN       EpochSeconds;
>    UINTN       Size;
>
>    if (Time == NULL) {
> @@ -75,13 +74,13 @@ LibGetTime (
>
>    // Get the epoch time from non-volatile storage
>    Size = sizeof (UINTN);
> -  ElapsedSeconds = 0;
> +  EpochSeconds = 0;
>    Status = EfiGetVariable (
>               (CHAR16 *)mEpochVariableName,
>               &gEfiCallerIdGuid,
>               NULL,
>               &Size,
> -             (VOID *)&ElapsedSeconds
> +             (VOID *)&EpochSeconds
>               );
>    // Fall back to compilation-time epoch if not set
>    if (EFI_ERROR (Status)) {
> @@ -93,7 +92,7 @@ LibGetTime (
>      // If you are attempting to use this library on such an environment, please
>      // contact the edk2 mailing list, so we can try to add support for it.
>      //
> -    ElapsedSeconds = BUILD_EPOCH;
> +    EpochSeconds = BUILD_EPOCH;
>      DEBUG ((
>        DEBUG_INFO,
>        "LibGetTime: %s non volatile variable was not found - Using compilation time epoch.\n",
> @@ -101,7 +100,7 @@ LibGetTime (
>        ));
>    }
>    Counter = GetPerformanceCounter ();
> -  ElapsedSeconds += DivU64x64Remainder (Counter, Freq, &Remainder);
> +  EpochSeconds += DivU64x64Remainder (Counter, Freq, &Remainder);
>
>    // Get the current time zone information from non-volatile storage
>    Size = sizeof (TimeZone);
> @@ -204,7 +203,7 @@ LibGetTime (
>      }
>    }
>
> -  EpochToEfiTime (ElapsedSeconds, Time);
> +  EpochToEfiTime (EpochSeconds, Time);
>
>    // Because we use the performance counter, we can fill the Nanosecond attribute
>    // provided that the remainder doesn't overflow 64-bit during multiplication.
> @@ -240,6 +239,9 @@ LibSetTime (
>    )
>  {
>    EFI_STATUS  Status;
> +  UINT64      Freq;
> +  UINT64      Counter;
> +  UINT64      Remainder;
>    UINTN       EpochSeconds;
>
>    if (!IsTimeValid (Time)) {
> @@ -249,16 +251,30 @@ LibSetTime (
>    EpochSeconds = EfiTimeToEpoch (Time);
>
>    // Adjust for the correct time zone, i.e. convert to UTC time zone
> -  if (Time->TimeZone != EFI_UNSPECIFIED_TIMEZONE) {
> +  if ((Time->TimeZone != EFI_UNSPECIFIED_TIMEZONE)
> +      && (EpochSeconds > Time->TimeZone * SEC_PER_MIN)) {
>      EpochSeconds -= Time->TimeZone * SEC_PER_MIN;
>    }
>
>    // Adjust for the correct period
> -  if ((Time->Daylight & EFI_TIME_IN_DAYLIGHT) == EFI_TIME_IN_DAYLIGHT) {
> +  if (((Time->Daylight & EFI_TIME_IN_DAYLIGHT) == EFI_TIME_IN_DAYLIGHT)
> +      && (EpochSeconds > SEC_PER_HOUR)) {
>      // Convert to un-adjusted time, i.e. fall back one hour
>      EpochSeconds -= SEC_PER_HOUR;
>    }
>
> +  // Use the performance counter to substract the number of seconds
> +  // since platform reset. Without this, setting time from the shell
> +  // and immediately reading it back would result in a forward time
> +  // offset, of the duration during which the platform has been up.
> +  Freq = GetPerformanceCounterProperties (NULL, NULL);
> +  if (Freq != 0) {
> +    Counter = GetPerformanceCounter ();
> +    if (EpochSeconds > DivU64x64Remainder (Counter, Freq, &Remainder)) {
> +      EpochSeconds -= DivU64x64Remainder (Counter, Freq, &Remainder);
> +    }
> +  }
> +
>    // Save the current time zone information into non-volatile storage
>    Status = EfiSetVariable (
>               (CHAR16 *)mTimeZoneVariableName,
> --
> 2.17.0.windows.1
>