Re: [PATCH v2 1/3] EmbeddedPkg/RealTimeClockRuntimeDxe: move common functionality into core

[Leif Lindholm <leif.lindholm@linaro.org>]

On Mon, Nov 06, 2017 at 06:20:36PM +0000, Ard Biesheuvel wrote:
> RealTimeClockRuntimeDxe defers the hardware/platform specific handling
> of reading/setting the hardware clock to RealTimeClockLib, but for
> unknown reasons, it also defers common functionality such as input
> validation and recording the timezone and DST settings (which are
> informational only and not managed by hardware)
> 
> This has led to a lot of duplication in implementations of RealTimeClockLib
> as well as TimeBaseLib, to the point where each library implementation
> has its own set of UEFI variables to record the timezone and DST settings.
> This makes little sense, and so let's update RealTimeClockRuntimeDxe now
> to allow future implementations to rely on the core driver to take care of
> these things.
> 
> Note that reading the timezone and DST settings occurs before calling into
> the library, so we can phase out this behavior gradually from library
> implementations in EDK2, edk2-platforms or out of tree.

Great consolidation, couple of minor questions/comments below:

> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClock.c             | 171 +++++++++++++++++++-
>  EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClockRuntimeDxe.inf |  11 +-
>  2 files changed, 171 insertions(+), 11 deletions(-)
> 
> diff --git a/EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClock.c b/EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClock.c
> index f1e067c0b59e..6b7cc876fa33 100644
> --- a/EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClock.c
> +++ b/EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClock.c
> @@ -1,10 +1,8 @@
>  /** @file
>    Implement EFI RealTimeClock runtime services via RTC Lib.
>  
> -  Currently this driver does not support runtime virtual calling.
> -
> -
>    Copyright (c) 2008 - 2009, Apple Inc. All rights reserved.<BR>
> +  Copyright (c) 2017, Linaro, Ltd. All rights reserved.<BR>
>  
>    This program and the accompanying materials
>    are licensed and made available under the terms and conditions of the BSD License
> @@ -17,14 +15,116 @@
>  **/
>  
>  #include <PiDxe.h>
> +#include <Library/DebugLib.h>
> +#include <Library/RealTimeClockLib.h>
>  #include <Library/UefiLib.h>
>  #include <Library/UefiBootServicesTableLib.h>
> -#include <Library/RealTimeClockLib.h>
> +#include <Library/UefiRuntimeLib.h>
>  #include <Protocol/RealTimeClock.h>
>  
>  EFI_HANDLE  mHandle = NULL;
>  
> +//
> +// These values can be set by SetTime () and need to be returned by GetTime ()
> +// but cannot usually be kept by the RTC hardware, so we store them in a UEFI
> +// variable instead.
> +//
> +typedef struct {
> +  INT16           TimeZone;
> +  UINT8           Daylight;
> +} NON_VOLATILE_TIME_SETTINGS;
> +
> +STATIC CONST CHAR16 mTimeSettingsVariableName[] = L"RtcTimeSettings";
> +STATIC NON_VOLATILE_TIME_SETTINGS mTimeSettings;
> +
> +STATIC
> +BOOLEAN
> +IsValidTimeZone (
> +  IN  INT16  TimeZone
> +  )
> +{
> +  return TimeZone == EFI_UNSPECIFIED_TIMEZONE ||
> +         (TimeZone >= -1440 && TimeZone <= 1440);
> +}
> +
> +STATIC
> +BOOLEAN
> +IsValidDaylight (
> +  IN  INT8  Daylight
> +  )
> +{
> +  return Daylight == 0 ||
> +         Daylight == EFI_TIME_ADJUST_DAYLIGHT ||
> +         Daylight == (EFI_TIME_ADJUST_DAYLIGHT | EFI_TIME_IN_DAYLIGHT);
> +}
>  
> +STATIC
> +BOOLEAN
> +EFIAPI
> +IsLeapYear (
> +  IN EFI_TIME   *Time
> +  )
> +{
> +  if (Time->Year % 4 == 0) {
> +    if (Time->Year % 100 == 0) {
> +      if (Time->Year % 400 == 0) {
> +        return TRUE;
> +      } else {
> +        return FALSE;
> +      }
> +    } else {
> +      return TRUE;
> +    }
> +  } else {
> +    return FALSE;
> +  }
> +}
> +
> +STATIC CONST INTN mDayOfMonth[12] = {
> +  31, 29, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31
> +};
> +
> +STATIC
> +BOOLEAN
> +EFIAPI
> +IsDayValid (
> +  IN  EFI_TIME  *Time
> +  )
> +{
> +  ASSERT (Time->Day >= 1);
> +  ASSERT (Time->Day <= mDayOfMonth[Time->Month - 1]);
> +  ASSERT (Time->Month != 2 || (IsLeapYear (Time) && Time->Day <= 29));

Is this me getting confused by basic logic operations, or should the
above be something like
   ASSERT (Time->Month != 2 || (Time->Day <= (28 + IsLeapYear (Time) ? 1 : 0)));
?

> +
> +  if (Time->Day < 1 ||
> +      Time->Day > mDayOfMonth[Time->Month - 1] ||
> +      (Time->Month == 2 && (!IsLeapYear (Time) && Time->Day > 28))) {
> +    return FALSE;
> +  }
> +  return TRUE;
> +}
> +
> +STATIC
> +BOOLEAN
> +EFIAPI
> +IsTimeValid(
> +  IN EFI_TIME *Time
> +  )
> +{
> +  // Check the input parameters are within the range specified by UEFI
> +  if (Time->Year   < 1900               ||
> +      Time->Year   > 9999               ||
> +      Time->Month  < 1                  ||
> +      Time->Month  > 12                 ||
> +      !IsDayValid (Time)                ||
> +      Time->Hour   > 23                 ||
> +      Time->Minute > 59                 ||
> +      Time->Second > 59                 ||
> +      !IsValidTimeZone (Time->TimeZone) ||
> +      !IsValidDaylight (Time->Daylight)) {
> +    return FALSE;
> +  }
> +  return TRUE;
> +}
>  
>  /**
>    Returns the current time and date information, and the time-keeping capabilities
> @@ -43,9 +143,20 @@ EFI_STATUS
>  EFIAPI
>  GetTime (
>    OUT EFI_TIME                *Time,
> -  OUT  EFI_TIME_CAPABILITIES  *Capabilities
> +  OUT EFI_TIME_CAPABILITIES   *Capabilities
>    )
>  {
> +  if (Time == NULL) {
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  //
> +  // Set these first so the RealTimeClockLib implementation
> +  // can override them based on its own settings.
> +  //
> +  Time->TimeZone = mTimeSettings.TimeZone;
> +  Time->Daylight = mTimeSettings.Daylight;
> +
>    return LibGetTime (Time, Capabilities);
>  }
>  
> @@ -67,7 +178,41 @@ SetTime (
>    IN EFI_TIME                *Time
>    )
>  {
> -  return LibSetTime (Time);
> +  EFI_STATUS        Status;
> +  BOOLEAN           TimeSettingsChanged;
> +
> +  if (Time == NULL || !IsTimeValid (Time)) {
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  TimeSettingsChanged = FALSE;
> +  if (mTimeSettings.TimeZone != Time->TimeZone ||
> +      mTimeSettings.Daylight != Time->Daylight) {
> +
> +    mTimeSettings.TimeZone = Time->TimeZone;
> +    mTimeSettings.Daylight = Time->Daylight;
> +    TimeSettingsChanged = TRUE;
> +  }
> +
> +  Status = LibSetTime (Time);
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  if (TimeSettingsChanged) {
> +    Status = EfiSetVariable (
> +               (CHAR16 *)mTimeSettingsVariableName,
> +               &gEfiCallerIdGuid,
> +               EFI_VARIABLE_NON_VOLATILE |
> +               EFI_VARIABLE_BOOTSERVICE_ACCESS |
> +               EFI_VARIABLE_RUNTIME_ACCESS,
> +               sizeof (mTimeSettings),
> +               (VOID *)&mTimeSettings);
> +    if (EFI_ERROR (Status)) {
> +      return EFI_DEVICE_ERROR;
> +    }
> +  }
> +  return EFI_SUCCESS;
>  }
>  
>  
> @@ -138,12 +283,26 @@ InitializeRealTimeClock (
>    )
>  {
>    EFI_STATUS  Status;
> +  UINTN       Size;
>  
>    Status = LibRtcInitialize (ImageHandle, SystemTable);
>    if (EFI_ERROR (Status)) {
>      return Status;
>    }
>  
> +  Size = sizeof (mTimeSettings);
> +  Status = EfiGetVariable ((CHAR16 *)mTimeSettingsVariableName,
> +                  &gEfiCallerIdGuid, NULL, &Size, (VOID *)&mTimeSettings);

Funky indentation?

/
    Leif

> +  if (EFI_ERROR (Status) ||
> +      !IsValidTimeZone (mTimeSettings.TimeZone) ||
> +      !IsValidDaylight (mTimeSettings.Daylight)) {
> +    DEBUG ((DEBUG_WARN, "%a: using default timezone/daylight settings\n",
> +      __FUNCTION__));
> +
> +    mTimeSettings.TimeZone = EFI_UNSPECIFIED_TIMEZONE;
> +    mTimeSettings.Daylight = 0;
> +  }
> +
>    SystemTable->RuntimeServices->GetTime       = GetTime;
>    SystemTable->RuntimeServices->SetTime       = SetTime;
>    SystemTable->RuntimeServices->GetWakeupTime = GetWakeupTime;
> diff --git a/EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClockRuntimeDxe.inf b/EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClockRuntimeDxe.inf
> index 186d4610bd42..d0520392f145 100644
> --- a/EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClockRuntimeDxe.inf
> +++ b/EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClockRuntimeDxe.inf
> @@ -1,8 +1,8 @@
>  #/** @file
> -# Reset Architectural Protocol Driver as defined in PI
> +#  Real Time Clock Architectural Protocol Driver as defined in PI
>  #
> -# This Reset module simulates system reset by process exit on NT.
> -# Copyright (c) 2006 - 2007, Intel Corporation. All rights reserved.<BR>
> +#  Copyright (c) 2006 - 2007, Intel Corporation. All rights reserved.<BR>
> +#  Copyright (c) 2017, Linaro, Ltd. All rights reserved.<BR>
>  #
>  #  This program and the accompanying materials
>  #  are licensed and made available under the terms and conditions of the BSD License
> @@ -31,10 +31,11 @@ [Packages]
>    EmbeddedPkg/EmbeddedPkg.dec
>  
>  [LibraryClasses]
> -  UefiBootServicesTableLib
> -  UefiDriverEntryPoint
>    DebugLib
>    RealTimeClockLib
> +  UefiBootServicesTableLib
> +  UefiDriverEntryPoint
> +  UefiRuntimeLib
>  
>  [Protocols]
>    gEfiRealTimeClockArchProtocolGuid
> -- 
> 2.11.0
>