On 09/08/20 23:04, Vladimir Olovyannikov wrote:
>> -----Original Message-----
>> From: Laszlo Ersek <ler...@redhat.com>
>> Sent: Monday, September 7, 2020 2:37 AM
>> To: Vladimir Olovyannikov <vladimir.olovyanni...@broadcom.com>;
>> Rabeda, Maciej <maciej.rab...@linux.intel.com>; devel@edk2.groups.io;
>> Zhichao Gao <zhichao....@intel.com>; Ray Ni <ray...@intel.com>
>> Cc: Samer El-Haj-Mahmoud <samer.el-haj-mahm...@arm.com>; Jiaxin Wu
>> <jiaxin...@intel.com>; Siyuan Fu <siyuan...@intel.com>; Liming Gao
>> <liming....@intel.com>; Nd <n...@arm.com>
>> Subject: Re: [PATCH v10 1/1] ShellPkg/DynamicCommand: add
>> HttpDynamicCommand
>>
>> On 09/04/20 19:55, Vladimir Olovyannikov wrote:
>>
>>> There is also another issue with the TimebaseLib: inconsistency in
>>> return values of the EfiTimeToEpoch (returns UINT32, should return
>>> UINTN, as Zhichao pointed out earlier in the previous
>>> HttpDynamicCommand patchset).
>>> If this one is fixed, I can just use the TimeBaseLib.h header for
>>> constants.
>>
>> Consuming TimeBaseLib.h in this patch would be really nice.
> OK, if this can be fixed, I will definitely use TimeBaseLib.h header for
> constants, and will drop
> duplicate definitions in Http.c/Http.h
>>
>> There are two EfiTimeToEpoch() call sites in edk2:
>>
>> ArmPlatformPkg/Library/PL031RealTimeClockLib/PL031RealTimeClockLib.c
>> EmbeddedPkg/Library/VirtualRealTimeClockLib/VirtualRealTimeClockLib.c
>>
>> The latter stores the return value in a UINTN variable, so that seems OK.
>> The
>> former is a bit messier, but it seems to ensure that the result fits in 32
>> bits for
>> HW reasons anyway:
>>
>> // Because the PL031 is a 32-bit counter counting seconds,
>> // the maximum time span is just over 136 years.
>> // Time is stored in Unix Epoch format, so it starts in 1970,
>> // Therefore it can not exceed the year 2106.
>> if ((Time->Year < 1970) || (Time->Year >= 2106)) {
>> return EFI_UNSUPPORTED;
>> }
>> ...
>> EpochSeconds = EfiTimeToEpoch (Time);
>> ...
>> MmioWrite32 (mPL031RtcBase + PL031_RTC_LR_LOAD_REGISTER,
>> EpochSeconds);
>>
>> So I think we'd need two patches:
>>
>> (1) add an explicit (UINT32) cast to the EfiTimeToEpoch() call in
>> "ArmPlatformPkg/Library/PL031RealTimeClockLib/PL031RealTimeClockLib.c",
>>
>> (2) change the return value to UINTN in
>> "EmbeddedPkg/Include/Library/TimeBaseLib.h" and
>> "EmbeddedPkg/Library/TimeBaseLib/TimeBaseLib.c".
>>
>> Hmm wait... There are five more call sites in edk2-platforms. :( OK, I
>> give up
>> here. Sorry.
> OK, so what are the next steps, what do you suggest?
You'd have to audit, and if necessary, clean up, the EfiTimeToEpoch()
call sites in edk2-platforms. And you'd need to establish a global order
between the patch series (plural) such that both edk2 and edk2-platforms
should build at any stage across those series.
Thanks
Laszlo
> I saw today that unused macros like SEC_PER_MONTH, etc. were removed from
> TimeBaseLib.h.
>
> Thank you,
> Vladimir
>>
>> Laszlo
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#65158): https://edk2.groups.io/g/devel/message/65158
Mute This Topic: https://groups.io/mt/76576293/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
