[PATCH v2 0/3] ArmPlatformPkg EmbeddedPkg: consolidate shared RTC functionality

[PATCH v2 0/3] ArmPlatformPkg EmbeddedPkg: consolidate shared RTC functionality

[Ard Biesheuvel]

This moves input validation and recording of the DST and timezone settings
(which cannot usually be done by the hardware) into the core RTC driver in
EmbeddedPkg, and removes it from one of the RealTimeClockLib implementations,
the one for the ARM PL031.

v2: split PL031 into two
    reinstate (but fix) the PL031 timezone handling (after reading and even
    understanding the original code)

Ard Biesheuvel (3):
  EmbeddedPkg/RealTimeClockRuntimeDxe: move common functionality into
    core
  ArmPlatformPkg/PL031RealTimeClockLib: remove validation and DST
    handling
  ArmPlatformPkg/PL031RealTimeClockLib: ignore DST setting when timezone
    is set

 ArmPlatformPkg/Library/PL031RealTimeClockLib/PL031RealTimeClockLib.c | 194 ++------------------
 EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClock.c                  | 171 ++++++++++++++++-
 EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClockRuntimeDxe.inf      |  11 +-
 3 files changed, 188 insertions(+), 188 deletions(-)

-- 
2.11.0

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

[Ard Biesheuvel]

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.

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

[PATCH v2 2/3] ArmPlatformPkg/PL031RealTimeClockLib: remove validation and DST handling

[Ard Biesheuvel]

This library, which is intended to encapsulate the hardware specifics
of the ARM PL031 RTC, also implements its own input validation routines
and record the timezone and DST settings in its own set of EFI variables.

This functionality has recently been added to the core driver, so let's
remove it here.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 ArmPlatformPkg/Library/PL031RealTimeClockLib/PL031RealTimeClockLib.c | 186 ++------------------
 1 file changed, 15 insertions(+), 171 deletions(-)

diff --git a/ArmPlatformPkg/Library/PL031RealTimeClockLib/PL031RealTimeClockLib.c b/ArmPlatformPkg/Library/PL031RealTimeClockLib/PL031RealTimeClockLib.c
index 41ebcb95ab85..f1eb0deb3249 100644
--- a/ArmPlatformPkg/Library/PL031RealTimeClockLib/PL031RealTimeClockLib.c
+++ b/ArmPlatformPkg/Library/PL031RealTimeClockLib/PL031RealTimeClockLib.c
@@ -40,8 +40,6 @@
 
 #include <ArmPlatform.h>
 
-STATIC CONST CHAR16           mTimeZoneVariableName[] = L"PL031RtcTimeZone";
-STATIC CONST CHAR16           mDaylightVariableName[] = L"PL031RtcDaylight";
 STATIC BOOLEAN                mPL031Initialized = FALSE;
 STATIC EFI_EVENT              mRtcVirtualAddrChangeEvent;
 STATIC UINTN                  mPL031RtcBase;
@@ -134,15 +132,12 @@ LibGetTime (
 {
   EFI_STATUS  Status = EFI_SUCCESS;
   UINT32      EpochSeconds;
-  INT16       TimeZone;
-  UINT8       Daylight;
-  UINTN       Size;
 
   // Initialize the hardware if not already done
   if (!mPL031Initialized) {
     Status = InitializePL031 ();
     if (EFI_ERROR (Status)) {
-      goto EXIT;
+      return Status;
     }
   }
 
@@ -156,7 +151,7 @@ LibGetTime (
     Status = EFI_SUCCESS;
   } else if (EFI_ERROR (Status)) {
     // Battery backed up hardware RTC exists but could not be read due to error. Abort.
-    goto EXIT;
+    return Status;
   } else {
     // Battery backed up hardware RTC exists and we read the time correctly from it.
     // Now sync the PL031 to the new time.
@@ -165,107 +160,18 @@ LibGetTime (
 
   // Ensure Time is a valid pointer
   if (Time == NULL) {
-    Status = EFI_INVALID_PARAMETER;
-    goto EXIT;
+    return EFI_INVALID_PARAMETER;
   }
 
-  // Get the current time zone information from non-volatile storage
-  Size = sizeof (TimeZone);
-  Status = EfiGetVariable (
-                  (CHAR16 *)mTimeZoneVariableName,
-                  &gEfiCallerIdGuid,
-                  NULL,
-                  &Size,
-                  (VOID *)&TimeZone
-                  );
-
-  if (EFI_ERROR (Status)) {
-    ASSERT(Status != EFI_INVALID_PARAMETER);
-    ASSERT(Status != EFI_BUFFER_TOO_SMALL);
-
-    if (Status != EFI_NOT_FOUND)
-      goto EXIT;
-
-    // The time zone variable does not exist in non-volatile storage, so create it.
-    Time->TimeZone = EFI_UNSPECIFIED_TIMEZONE;
-    // Store it
-    Status = EfiSetVariable (
-                    (CHAR16 *)mTimeZoneVariableName,
-                    &gEfiCallerIdGuid,
-                    EFI_VARIABLE_NON_VOLATILE | EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS,
-                    Size,
-                    (VOID *)&(Time->TimeZone)
-                    );
-    if (EFI_ERROR (Status)) {
-      DEBUG ((
-        EFI_D_ERROR,
-        "LibGetTime: Failed to save %s variable to non-volatile storage, Status = %r\n",
-        mTimeZoneVariableName,
-        Status
-        ));
-      goto EXIT;
-    }
-  } else {
-    // Got the time zone
-    Time->TimeZone = TimeZone;
-
-    // Check TimeZone bounds:   -1440 to 1440 or 2047
-    if (((Time->TimeZone < -1440) || (Time->TimeZone > 1440))
-        && (Time->TimeZone != EFI_UNSPECIFIED_TIMEZONE)) {
-      Time->TimeZone = EFI_UNSPECIFIED_TIMEZONE;
-    }
-
-    // Adjust for the correct time zone
-    if (Time->TimeZone != EFI_UNSPECIFIED_TIMEZONE) {
-      EpochSeconds += Time->TimeZone * SEC_PER_MIN;
-    }
+  // Adjust for the correct time zone
+  if (Time->TimeZone != EFI_UNSPECIFIED_TIMEZONE) {
+    EpochSeconds += Time->TimeZone * SEC_PER_MIN;
   }
 
-  // Get the current daylight information from non-volatile storage
-  Size = sizeof (Daylight);
-  Status = EfiGetVariable (
-                  (CHAR16 *)mDaylightVariableName,
-                  &gEfiCallerIdGuid,
-                  NULL,
-                  &Size,
-                  (VOID *)&Daylight
-                  );
-
-  if (EFI_ERROR (Status)) {
-    ASSERT(Status != EFI_INVALID_PARAMETER);
-    ASSERT(Status != EFI_BUFFER_TOO_SMALL);
-
-    if (Status != EFI_NOT_FOUND)
-      goto EXIT;
-
-    // The daylight variable does not exist in non-volatile storage, so create it.
-    Time->Daylight = 0;
-    // Store it
-    Status = EfiSetVariable (
-                    (CHAR16 *)mDaylightVariableName,
-                    &gEfiCallerIdGuid,
-                    EFI_VARIABLE_NON_VOLATILE | EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS,
-                    Size,
-                    (VOID *)&(Time->Daylight)
-                    );
-    if (EFI_ERROR (Status)) {
-      DEBUG ((
-        EFI_D_ERROR,
-        "LibGetTime: Failed to save %s variable to non-volatile storage, Status = %r\n",
-        mDaylightVariableName,
-        Status
-        ));
-      goto EXIT;
-    }
-  } else {
-    // Got the daylight information
-    Time->Daylight = Daylight;
-
-    // Adjust for the correct period
-    if ((Time->Daylight & EFI_TIME_IN_DAYLIGHT) == EFI_TIME_IN_DAYLIGHT) {
-      // Convert to adjusted time, i.e. spring forwards one hour
-      EpochSeconds += SEC_PER_HOUR;
-    }
+  // Adjust for the correct period
+  if ((Time->Daylight & EFI_TIME_IN_DAYLIGHT) == EFI_TIME_IN_DAYLIGHT) {
+    // Convert to adjusted time, i.e. spring forwards one hour
+    EpochSeconds += SEC_PER_HOUR;
   }
 
   // Convert from internal 32-bit time to UEFI time
@@ -281,8 +187,7 @@ LibGetTime (
     Capabilities->SetsToZero  = FALSE;
   }
 
-  EXIT:
-  return Status;
+  return EFI_SUCCESS;
 }
 
 
@@ -305,37 +210,19 @@ LibSetTime (
   EFI_STATUS  Status;
   UINTN       EpochSeconds;
 
-  // 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  ) ||
-       (Time->Nanosecond > 999999999) ||
-       (!((Time->TimeZone == EFI_UNSPECIFIED_TIMEZONE) || ((Time->TimeZone >= -1440) && (Time->TimeZone <= 1440)))) ||
-       (Time->Daylight & (~(EFI_TIME_ADJUST_DAYLIGHT | EFI_TIME_IN_DAYLIGHT)))
-    ) {
-    Status = EFI_INVALID_PARAMETER;
-    goto EXIT;
-  }
-
   // Because the PL031 is a 32-bit counter counting seconds,
   // the maximum time span is just over 136 years.
   // Time is stored in Unix Epoch format, so it starts in 1970,
   // Therefore it can not exceed the year 2106.
   if ((Time->Year < 1970) || (Time->Year >= 2106)) {
-    Status = EFI_UNSUPPORTED;
-    goto EXIT;
+    return EFI_UNSUPPORTED;
   }
 
   // Initialize the hardware if not already done
   if (!mPL031Initialized) {
     Status = InitializePL031 ();
     if (EFI_ERROR (Status)) {
-      goto EXIT;
+      return Status;
     }
   }
 
@@ -346,8 +233,6 @@ LibSetTime (
     EpochSeconds -= Time->TimeZone * SEC_PER_MIN;
   }
 
-  // TODO: Automatic Daylight activation
-
   // Adjust for the correct period
   if ((Time->Daylight & EFI_TIME_IN_DAYLIGHT) == EFI_TIME_IN_DAYLIGHT) {
     // Convert to un-adjusted time, i.e. fall back one hour
@@ -364,54 +249,13 @@ LibSetTime (
   Status = ArmPlatformSysConfigSet (SYS_CFG_RTC, EpochSeconds);
   if ((EFI_ERROR (Status)) && (Status != EFI_UNSUPPORTED)){
     // Any status message except SUCCESS and UNSUPPORTED indicates a hardware failure.
-    goto EXIT;
+    return Status;
   }
 
-
   // Set the PL031
   MmioWrite32 (mPL031RtcBase + PL031_RTC_LR_LOAD_REGISTER, EpochSeconds);
 
-  // The accesses to Variable Services can be very slow, because we may be writing to Flash.
-  // Do this after having set the RTC.
-
-  // Save the current time zone information into non-volatile storage
-  Status = EfiSetVariable (
-                  (CHAR16 *)mTimeZoneVariableName,
-                  &gEfiCallerIdGuid,
-                  EFI_VARIABLE_NON_VOLATILE | EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS,
-                  sizeof (Time->TimeZone),
-                  (VOID *)&(Time->TimeZone)
-                  );
-  if (EFI_ERROR (Status)) {
-      DEBUG ((
-        EFI_D_ERROR,
-        "LibSetTime: Failed to save %s variable to non-volatile storage, Status = %r\n",
-        mTimeZoneVariableName,
-        Status
-        ));
-    goto EXIT;
-  }
-
-  // Save the current daylight information into non-volatile storage
-  Status = EfiSetVariable (
-                  (CHAR16 *)mDaylightVariableName,
-                  &gEfiCallerIdGuid,
-                  EFI_VARIABLE_NON_VOLATILE | EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS,
-                  sizeof(Time->Daylight),
-                  (VOID *)&(Time->Daylight)
-                  );
-  if (EFI_ERROR (Status)) {
-    DEBUG ((
-      EFI_D_ERROR,
-      "LibSetTime: Failed to save %s variable to non-volatile storage, Status = %r\n",
-      mDaylightVariableName,
-      Status
-      ));
-    goto EXIT;
-  }
-
-  EXIT:
-  return Status;
+  return EFI_SUCCESS;
 }
 
 
-- 
2.11.0

[PATCH v2 3/3] ArmPlatformPkg/PL031RealTimeClockLib: ignore DST setting when timezone is set

[Ard Biesheuvel]

According to the UEFI spec, the timezone setting which the platform needs
to record in addition to the actual date and time already reflects the
current DST setting. In other words, moving the clock from standard time
to daylight saving time also involves adding or subtracting 60 minutes
from the timezone setting, as well as flicking the EFI_TIME_IN_DAYLIGHT
bit in the DST setting.

This means we need to disregard the DST setting if the timezone is
specified, and only add or subtract the additional hour if we are on
local time.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 ArmPlatformPkg/Library/PL031RealTimeClockLib/PL031RealTimeClockLib.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/ArmPlatformPkg/Library/PL031RealTimeClockLib/PL031RealTimeClockLib.c b/ArmPlatformPkg/Library/PL031RealTimeClockLib/PL031RealTimeClockLib.c
index f1eb0deb3249..459dcc0a0519 100644
--- a/ArmPlatformPkg/Library/PL031RealTimeClockLib/PL031RealTimeClockLib.c
+++ b/ArmPlatformPkg/Library/PL031RealTimeClockLib/PL031RealTimeClockLib.c
@@ -164,12 +164,10 @@ LibGetTime (
   }
 
   // Adjust for the correct time zone
+  // The timezone setting also reflects the DST setting of the clock
   if (Time->TimeZone != EFI_UNSPECIFIED_TIMEZONE) {
     EpochSeconds += Time->TimeZone * SEC_PER_MIN;
-  }
-
-  // Adjust for the correct period
-  if ((Time->Daylight & EFI_TIME_IN_DAYLIGHT) == EFI_TIME_IN_DAYLIGHT) {
+  } else if ((Time->Daylight & EFI_TIME_IN_DAYLIGHT) == EFI_TIME_IN_DAYLIGHT) {
     // Convert to adjusted time, i.e. spring forwards one hour
     EpochSeconds += SEC_PER_HOUR;
   }
@@ -229,12 +227,10 @@ LibSetTime (
   EpochSeconds = EfiTimeToEpoch (Time);
 
   // Adjust for the correct time zone, i.e. convert to UTC time zone
+  // The timezone setting also reflects the DST setting of the clock
   if (Time->TimeZone != EFI_UNSPECIFIED_TIMEZONE) {
     EpochSeconds -= Time->TimeZone * SEC_PER_MIN;
-  }
-
-  // Adjust for the correct period
-  if ((Time->Daylight & EFI_TIME_IN_DAYLIGHT) == EFI_TIME_IN_DAYLIGHT) {
+  } else if ((Time->Daylight & EFI_TIME_IN_DAYLIGHT) == EFI_TIME_IN_DAYLIGHT) {
     // Convert to un-adjusted time, i.e. fall back one hour
     EpochSeconds -= SEC_PER_HOUR;
   }
-- 
2.11.0

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

[Leif Lindholm]

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
> 

Re: [PATCH v2 2/3] ArmPlatformPkg/PL031RealTimeClockLib: remove validation and DST handling

[Leif Lindholm]

On Mon, Nov 06, 2017 at 06:20:37PM +0000, Ard Biesheuvel wrote:
> This library, which is intended to encapsulate the hardware specifics
> of the ARM PL031 RTC, also implements its own input validation routines
> and record the timezone and DST settings in its own set of EFI variables.
> 
> This functionality has recently been added to the core driver, so let's
> remove it here.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

Re: [PATCH v2 3/3] ArmPlatformPkg/PL031RealTimeClockLib: ignore DST setting when timezone is set

[Leif Lindholm]

On Mon, Nov 06, 2017 at 06:20:38PM +0000, Ard Biesheuvel wrote:
> According to the UEFI spec, the timezone setting which the platform needs
> to record in addition to the actual date and time already reflects the
> current DST setting. In other words, moving the clock from standard time
> to daylight saving time also involves adding or subtracting 60 minutes
> from the timezone setting, as well as flicking the EFI_TIME_IN_DAYLIGHT
> bit in the DST setting.
> 
> This means we need to disregard the DST setting if the timezone is
> specified, and only add or subtract the additional hour if we are on
> local time.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

Assuming I understand the above correctly, and page 259/260 in 2.7a:
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

> ---
>  ArmPlatformPkg/Library/PL031RealTimeClockLib/PL031RealTimeClockLib.c | 12 ++++--------
>  1 file changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/ArmPlatformPkg/Library/PL031RealTimeClockLib/PL031RealTimeClockLib.c b/ArmPlatformPkg/Library/PL031RealTimeClockLib/PL031RealTimeClockLib.c
> index f1eb0deb3249..459dcc0a0519 100644
> --- a/ArmPlatformPkg/Library/PL031RealTimeClockLib/PL031RealTimeClockLib.c
> +++ b/ArmPlatformPkg/Library/PL031RealTimeClockLib/PL031RealTimeClockLib.c
> @@ -164,12 +164,10 @@ LibGetTime (
>    }
>  
>    // Adjust for the correct time zone
> +  // The timezone setting also reflects the DST setting of the clock
>    if (Time->TimeZone != EFI_UNSPECIFIED_TIMEZONE) {
>      EpochSeconds += Time->TimeZone * SEC_PER_MIN;
> -  }
> -
> -  // Adjust for the correct period
> -  if ((Time->Daylight & EFI_TIME_IN_DAYLIGHT) == EFI_TIME_IN_DAYLIGHT) {
> +  } else if ((Time->Daylight & EFI_TIME_IN_DAYLIGHT) == EFI_TIME_IN_DAYLIGHT) {
>      // Convert to adjusted time, i.e. spring forwards one hour
>      EpochSeconds += SEC_PER_HOUR;
>    }
> @@ -229,12 +227,10 @@ LibSetTime (
>    EpochSeconds = EfiTimeToEpoch (Time);
>  
>    // Adjust for the correct time zone, i.e. convert to UTC time zone
> +  // The timezone setting also reflects the DST setting of the clock
>    if (Time->TimeZone != EFI_UNSPECIFIED_TIMEZONE) {
>      EpochSeconds -= Time->TimeZone * SEC_PER_MIN;
> -  }
> -
> -  // Adjust for the correct period
> -  if ((Time->Daylight & EFI_TIME_IN_DAYLIGHT) == EFI_TIME_IN_DAYLIGHT) {
> +  } else if ((Time->Daylight & EFI_TIME_IN_DAYLIGHT) == EFI_TIME_IN_DAYLIGHT) {
>      // Convert to un-adjusted time, i.e. fall back one hour
>      EpochSeconds -= SEC_PER_HOUR;
>    }
> -- 
> 2.11.0
> 

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

[Ard Biesheuvel]

On 9 November 2017 at 16:26, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> 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)));
> ?
>

Yeah that does like dodgy. This actuall originates from TimeBaseLib,
which I would like to phase out as well (or reduce to the
EfiToEpochSeconds routines)

I think this should be

ASSERT (Time->Month != 2 || IsLeapYear (Time) || Time->Day <= 28);


>> +
>> +  if (Time->Day < 1 ||
>> +      Time->Day > mDayOfMonth[Time->Month - 1] ||
>> +      (Time->Month == 2 && (!IsLeapYear (Time) && Time->Day > 28))) {

Here, we should probably drop the inner parens, which will make the
expression more obviously the negation of the one above.

>> +    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?
>

Yeah. Will fix.

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