From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2a00:1450:400c:c09::243; helo=mail-wm0-x243.google.com; envelope-from=leif.lindholm@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-wm0-x243.google.com (mail-wm0-x243.google.com [IPv6:2a00:1450:400c:c09::243]) (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 A377F2034D821 for ; Thu, 9 Nov 2017 08:22:04 -0800 (PST) Received: by mail-wm0-x243.google.com with SMTP id s66so18208396wmf.5 for ; Thu, 09 Nov 2017 08:26:06 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=8j/ERd8m/Q9W8SmXSpv019+YUn3/kpPY9TuuK7/B8gs=; b=bqQagutP6cQ2rv4r+o10/nemEmtL9ueRNfd3YHwI0fzUrQkm2D+VnifI19JOAXNdqN gkM1DpW2iPppa/XcbUPB1WhvVnmwLUR9Oqhdr08zPhtPeY4BiahyknervMXn9I1JcwXG zFol1lduWwuglvziRnVU2SLpuGsqE2b9ps440= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=8j/ERd8m/Q9W8SmXSpv019+YUn3/kpPY9TuuK7/B8gs=; b=FqMKWomn/s3dumR05hfGmYVFqwpkgLtHVnZYWp4oYyC+Lezf5mcKyS25Q/Pg6VmkEp KuHSnF+Q4gFuyet1RtB5XfsibXpc47cXfLneyCYlO0Y6si9S8lVn19ZPqsy+/vQdvsXF Qr/ooK78DmxW++Q+EBt0ax61iB4sRKkyNaMdpBctC3VuR9Y4BIEnGu/EyuopCqMbowYH KGtE0OUO8tAjj+AoA+fmQBvScfDiUIZpYEfChOdJLXTiO3l1qxLciciXnRo4dQWMa3AK FgyGDZKgwVdV5+2eiWqYoMqqRt8AmDkjXulReNwiADLh0gRFy2QELJJo2KGN2/Re1SrV RqKg== X-Gm-Message-State: AJaThX5kJHcoQuCfRHKuVbe1xhLejf4WZSocLuZJxKvmB5vIWnF6/1Zp HLUaEN2zXeM8DxbtNcx49jAMvaidWFg= X-Google-Smtp-Source: AGs4zMb6slhx2MfK3scs0HF7kPsK3tN2U+RjfOnzfnAUW7TKWFc3kDCMhvRj6xtHHvYNVm7X7KKewQ== X-Received: by 10.28.148.15 with SMTP id w15mr313899wmd.140.1510244764733; Thu, 09 Nov 2017 08:26:04 -0800 (PST) Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id h3sm11755400wre.63.2017.11.09.08.26.03 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Thu, 09 Nov 2017 08:26:03 -0800 (PST) Date: Thu, 9 Nov 2017 16:26:02 +0000 From: Leif Lindholm To: Ard Biesheuvel Cc: edk2-devel@lists.01.org, udit.kumar@nxp.com Message-ID: <20171109162602.a5saxky2klnkytnd@bivouac.eciton.net> References: <20171106182038.16750-1-ard.biesheuvel@linaro.org> <20171106182038.16750-2-ard.biesheuvel@linaro.org> MIME-Version: 1.0 In-Reply-To: <20171106182038.16750-2-ard.biesheuvel@linaro.org> User-Agent: NeoMutt/20170113 (1.7.2) 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 16:22:05 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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))); ? > + > + 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.
> +# 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 >