From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:4001:c06::242; helo=mail-io0-x242.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-io0-x242.google.com (mail-io0-x242.google.com [IPv6:2607:f8b0:4001:c06::242]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id E77E72034A888 for ; Thu, 9 Nov 2017 13:19:02 -0800 (PST) Received: by mail-io0-x242.google.com with SMTP id n137so11393094iod.6 for ; Thu, 09 Nov 2017 13:23:04 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=wrYu/wkGHdEy8C97qPeHHzHebh9EyGfWVLIVj6hCuN0=; b=FwoVh2yLoSthNG2UkV7zM1VYr9J7f+3blDc5tkugsz1Ta33oJ8ryZtAOF1mY1JfEJy uCtHLgLfUc0FF7BSgXQdmsZYu6JFTTjZWuEGTJO+OdXzxkid7b328zP8aZmOEC+ZUAOO Lm8626mCt8+StLCnpfx6UDOPowV5CV5jeY58k= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=wrYu/wkGHdEy8C97qPeHHzHebh9EyGfWVLIVj6hCuN0=; b=efabHPxG+WWzDx/ENVUn+2BWBbixWz7OgnfT6yGeLD7qmhMSHD2icr6e79x4oKzQGe ZS564zm7i+WnFmLXYRvPyh7vi9jCpCueBGnPmZa8EsRpErSCf0hnE0rWXam9BiHK8CYs pjnugdZxuqOwwjWFm/MO+N565ew4QjPDMWmdbJXzhs2D/XRgnNABit6ccLIZ+Teu1oR5 lZjbNW10F75ZWeCpAZBFUtYfszT2/5Wp5tp2O3T6+rpEthKHEFEzoZGh+Zt7xyDsp0LO VrJeYSyO0B71HVhflHnbqt9e91VuZxCyNlc9rRp6XdWT65p67tM3mgfElV1iCsW9mgvQ MirA== X-Gm-Message-State: AJaThX7K3H0qGmkdqUqXH5CAE55Smlq4NOTzVHBuSsoG9hGzkgijThFk a0Je1BZAqI8bht0yKuxJSttgOX6S5R7V3Nr/gJUJ/w== X-Google-Smtp-Source: AGs4zMbEoB/hg1TI+nKFGuxd4jnHcBDsaFv4oNr+MgX4rkaBXPkewxuDstLp+nPqhmlTi3kinipR8UCXHjg+7Awjuc4= X-Received: by 10.107.142.208 with SMTP id q199mr2362181iod.186.1510262584156; Thu, 09 Nov 2017 13:23:04 -0800 (PST) MIME-Version: 1.0 Received: by 10.107.131.167 with HTTP; Thu, 9 Nov 2017 13:23:03 -0800 (PST) In-Reply-To: <20171109162602.a5saxky2klnkytnd@bivouac.eciton.net> References: <20171106182038.16750-1-ard.biesheuvel@linaro.org> <20171106182038.16750-2-ard.biesheuvel@linaro.org> <20171109162602.a5saxky2klnkytnd@bivouac.eciton.net> From: Ard Biesheuvel Date: Thu, 9 Nov 2017 21:23:03 +0000 Message-ID: To: Leif Lindholm Cc: "edk2-devel@lists.01.org" , Udit Kumar Subject: Re: [PATCH v2 1/3] EmbeddedPkg/RealTimeClockRuntimeDxe: move common functionality into core X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 09 Nov 2017 21:19:03 -0000 Content-Type: text/plain; charset="UTF-8" On 9 November 2017 at 16:26, Leif Lindholm 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 >> --- >> 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.
>> + Copyright (c) 2017, Linaro, Ltd. All rights reserved.
>> >> 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 >> +#include >> +#include >> #include >> #include >> -#include >> +#include >> #include >> >> 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.
>> +# Copyright (c) 2006 - 2007, Intel Corporation. All rights reserved.
>> +# Copyright (c) 2017, Linaro, Ltd. All rights reserved.
>> # >> # 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 >>