Re: [PATCH 1/1] EmbeddedPkg/Library: Add VirtualRealTimeClockLib

[Pete Batard <pete@akeo.ie>]

On 2019-02-15 14:39, Ard Biesheuvel wrote:
> On Fri, 15 Feb 2019 at 11:07, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>>
>> On Tue, 12 Feb 2019 at 19:14, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>>>
>>> On Mon, Feb 04, 2019 at 12:47:36PM +0000, Pete Batard wrote:
>>>> This is designed to be used on platforms where a a real RTC is not
>>>> available and relies on an RtcEpochSeconds variable having been set or,
>>>> if that is not the case, falls back to using the epoch embedded at
>>>> compilation time.
>>>>
>>>> Note that, in order to keep things simple for the setting of the
>>>> compilation time variable, only GCC environments with UNIX-like shells
>>>> and where a 'date' command is available are meant to be supported for
>>>> now.
>>>>
>>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>>> Signed-off-by: Pete Batard <pete@akeo.ie>
>>>
>>> On the whole, this looks good to me.
>>> One addition we'll need, so that we can build this library standalone
>>> is an entry in EmbeddedPkg.dsc:
>>>
>>> diff --git a/EmbeddedPkg/EmbeddedPkg.dsc b/EmbeddedPkg/EmbeddedPkg.dsc
>>> index 4d9e6399d5..dc5040e611 100644
>>> --- a/EmbeddedPkg/EmbeddedPkg.dsc
>>> +++ b/EmbeddedPkg/EmbeddedPkg.dsc
>>> @@ -218,6 +218,7 @@ [Components.common]
>>>     EmbeddedPkg/Library/CoherentDmaLib/CoherentDmaLib.inf
>>>     EmbeddedPkg/Library/NonCoherentDmaLib/NonCoherentDmaLib.inf
>>>     EmbeddedPkg/Library/DxeDtPlatformDtbLoaderLibDefault/DxeDtPlatformDtbLoaderLibDefault.inf
>>> +  EmbeddedPkg/Library/VirtualRealTimeClockLib/VirtualRealTimeClockLib.inf
>>>     EmbeddedPkg/EmbeddedMonotonicCounter/EmbeddedMonotonicCounter.inf
>>>     EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClockRuntimeDxe.inf
>>>
>>> I don't have any strong opinions on either of Phil's suggestions, but
>>> if you could give some feedback on those and fold the above in, this
>>> could go in.
>>>
>>
>> WIth this addition
>>
>> Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>
>> Pushed as 1b261a705f94..64a17fadcb79
> 
> OK, there is a problem with this code:
> 
> +EFI_STATUS
> +EFIAPI
> +LibGetTime (
> +  OUT EFI_TIME               *Time,
> +  OUT EFI_TIME_CAPABILITIES  *Capabilities
> +  )
> +{
> +  EFI_STATUS  Status;
> +  UINT32      EpochSeconds;
> 
> EpochSeconds is declared here, and updated depending on time zone and
> DST settings. However, the resulting value is never used anywhere.

You're right.

Looks like I forgot to merge all the use of EpochSeconds into 
ElapsedSeconds, from the code I copy/pasted.

I'm very sorry about this, as it's something I should have picked up 
before sending this patch for review.
> It is not clear to me what the correct fix is, so Pete, could you
> please look into this?

I'll send a fix for this as soon as I have a chance. Thanks for pointing 
the mistake.

Regards,

/Pete