* [PATCH 0/1] EmbeddedPkg: Fix multiple Virtual RTC issues
@ 2019-02-25 23:52 Pete Batard
2019-02-25 23:52 ` [PATCH 1/1] " Pete Batard
0 siblings, 1 reply; 3+ messages in thread
From: Pete Batard @ 2019-02-25 23:52 UTC (permalink / raw)
To: edk2-devel
This patch fixes multiple issues in VirtualRealTimeClockLib.c:
1. Use of two separate variables (ElapsedSeconds and EpochSeconds) in
LibGetTime(), where only a single one should have been used.
2. Possible underflow while sustracting TZ/DST offsets in LibSetTime()
if the user sets a date that is very close to start of epoch.
3. Non substraction of elpased seconds since reset, which would lead to
the following behaviour in UEFI shell (assuming for this example that
exactly 5 minutes have elapsed since platform reset):
Shell> time 12:00:00
Shell> time
12:05:01 (LOCAL)
In other words, setting and immediately reading back the time would
result in the returned value being offset by the time since reset.
This last behaviour has been observed (and confirmed fixed) using an
RPi3 platform.
Regards,
/Pete
Pete Batard (1):
EmbeddedPkg: Fix multiple Virtual RTC issues
.../VirtualRealTimeClockLib.c | 34 ++++++++++++++-----
1 file changed, 25 insertions(+), 9 deletions(-)
--
2.17.0.windows.1
^ permalink raw reply [flat|nested] 3+ messages in thread* [PATCH 1/1] EmbeddedPkg: Fix multiple Virtual RTC issues 2019-02-25 23:52 [PATCH 0/1] EmbeddedPkg: Fix multiple Virtual RTC issues Pete Batard @ 2019-02-25 23:52 ` Pete Batard 2019-02-26 7:36 ` Ard Biesheuvel 0 siblings, 1 reply; 3+ messages in thread From: Pete Batard @ 2019-02-25 23:52 UTC (permalink / raw) To: edk2-devel 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> --- 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 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH 1/1] EmbeddedPkg: Fix multiple Virtual RTC issues 2019-02-25 23:52 ` [PATCH 1/1] " Pete Batard @ 2019-02-26 7:36 ` Ard Biesheuvel 0 siblings, 0 replies; 3+ messages in thread From: Ard Biesheuvel @ 2019-02-26 7:36 UTC (permalink / raw) To: Pete Batard; +Cc: edk2-devel@lists.01.org, Leif Lindholm 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 > ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-02-26 7:36 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2019-02-25 23:52 [PATCH 0/1] EmbeddedPkg: Fix multiple Virtual RTC issues Pete Batard 2019-02-25 23:52 ` [PATCH 1/1] " Pete Batard 2019-02-26 7:36 ` Ard Biesheuvel
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox