Re: [edk2-devel] [PATCH v10 1/1] ShellPkg/DynamicCommand: add HttpDynamicCommand

["Laszlo Ersek" <lersek@redhat.com>]

Hi Vladimir,

On 09/09/20 19:15, Vladimir Olovyannikov via groups.io wrote:
>> -----Original Message-----
>> From: Laszlo Ersek <lersek@redhat.com>
>> Sent: Wednesday, September 9, 2020 3:51 AM
>> To: Vladimir Olovyannikov <vladimir.olovyannikov@broadcom.com>;
>> Rabeda, Maciej <maciej.rabeda@linux.intel.com>; devel@edk2.groups.io;
>> Zhichao Gao <zhichao.gao@intel.com>; Ray Ni <ray.ni@intel.com>
>> Cc: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>; Jiaxin Wu
>> <jiaxin.wu@intel.com>; Siyuan Fu <siyuan.fu@intel.com>; Liming Gao
>> <liming.gao@intel.com>; Nd <nd@arm.com>
>> Subject: Re: [PATCH v10 1/1] ShellPkg/DynamicCommand: add
>> HttpDynamicCommand
>>
>> On 09/08/20 23:04, Vladimir Olovyannikov wrote:
>>>> -----Original Message-----
>>>> From: Laszlo Ersek <lersek@redhat.com>
>>>> Sent: Monday, September 7, 2020 2:37 AM
>>>> To: Vladimir Olovyannikov <vladimir.olovyannikov@broadcom.com>;
>>>> Rabeda, Maciej <maciej.rabeda@linux.intel.com>;
>> devel@edk2.groups.io;
>>>> Zhichao Gao <zhichao.gao@intel.com>; Ray Ni <ray.ni@intel.com>
>>>> Cc: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>; Jiaxin
>> Wu
>>>> <jiaxin.wu@intel.com>; Siyuan Fu <siyuan.fu@intel.com>; Liming Gao
>>>> <liming.gao@intel.com>; Nd <nd@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.

> I looked through the platforms, and all of them use UINT32 for
> EfiTimeToEpoch() return value, so IMHO it is sufficient to
> change EfiTimeToEpoch's variables to UINTN, and then return
> (UINT32)(EpochSeconds) in EfiTimeToEpoch().

This doesn't look like a good design to me. According to the lib class
header,

  EmbeddedPkg/Include/Library/TimeBaseLib.h

the current function prototype is

> /**
>   Converts EFI_TIME to Epoch seconds (elapsed since 1970 JANUARY 01, 00:00:00 UTC)
>  **/
> UINT32
> EFIAPI
> EfiTimeToEpoch (
>   IN  EFI_TIME  *Time
>   );

This interface has a "year 2106" problem even on 64-bit systems.

Now... honestly, I'm confused about this library:

- It does not clearly state what particular EFI_TIME values it intends
  to handle.

- It has a function called IsTimeValid().

- But it never calls the IsTimeValid() function itself, internally.

- The IsTimeValid() function is inconsistent. See:

> BOOLEAN
> EFIAPI
> IsTimeValid(
>   IN EFI_TIME *Time
>   )
> {
>   // Check the input parameters are within the range specified by UEFI
>   if ((Time->Year   < 2000) ||
>      (Time->Year   > 2099) ||
>      (Time->Month  < 1   ) ||
>      (Time->Month  > 12  ) ||
>      (!IsDayValid (Time)    ) ||
>      (Time->Hour   > 23  ) ||
>      (Time->Minute > 59  ) ||
>      (Time->Second > 59  ) ||
>      (Time->Nanosecond > 999999999) ||
>      (!((Time->TimeZone == EFI_UNSPECIFIED_TIMEZONE) || ((Time->TimeZone >= -1440) && (Time->TimeZone <= 1440)))) ||
>      (Time->Daylight & (~(EFI_TIME_ADJUST_DAYLIGHT | EFI_TIME_IN_DAYLIGHT)))
>   ) {
>     return FALSE;
>   }
> 
>   return TRUE;
> }

  The year limit 2099 is consistent with the "year 2106" limitation of
  EfiTimeToEpoch(), yes. But the comment is wrong! The comment says
  "Check the input parameters are within the range specified by UEFI" --
  but UEFI places an upper inclusive limit of *9999* on Time->Year.

- Edk2 has *another* implementation of EfiTimeToEpoch(), namely
  IsTimeValid(), namely in
  "EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClock.c". (It's a driver,
  not a library.) Compare:

> 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                 ||
>       Time->Nanosecond > 999999999      ||
>       !IsValidTimeZone (Time->TimeZone) ||
>       !IsValidDaylight (Time->Daylight)) {
>     return FALSE;
>   }
>   return TRUE;
> }

  *Slightly* different.

So what am I to make of the intents and goals of TimeBaseLib?

I think I'll just stop discussing TimeBaseLib :/

Thanks
Laszlo