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

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On Fri, 15 Feb 2019 at 15:57, Pete Batard <pete@akeo.ie> wrote:
>
> 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.

No need to worry about it, as long as you get it fixed :-)

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