From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [63.128.21.124]) by mx.groups.io with SMTP id smtpd.web12.15688.1599719124920158407 for ; Wed, 09 Sep 2020 23:25:25 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=c+jmLoF3; spf=pass (domain: redhat.com, ip: 63.128.21.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1599719124; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=uEiFG7xMEHoJuFp+ddP72u6KFIEK8NbMt8aDncsL+bI=; b=c+jmLoF36wzzCvbyMMKillvmnq25N4M3YvtK/lAF/Ll2FI8LeL9+eCvnTPo/GFcFECmvNH Calpm44uhE26U25BZmV+R5g8bIbAtNhdiVgd2aYWrcZE/mBsuD7gH7gWxonlSTASpOTRk0 evuBMsKuU49vGZfGrq9eCuTgJr6L2Kw= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-92-glmDvjOpNSK9wo5nGzbZUA-1; Thu, 10 Sep 2020 02:25:19 -0400 X-MC-Unique: glmDvjOpNSK9wo5nGzbZUA-1 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 24DBC1DE0E; Thu, 10 Sep 2020 06:25:18 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-112-181.ams2.redhat.com [10.36.112.181]) by smtp.corp.redhat.com (Postfix) with ESMTP id C409486583; Thu, 10 Sep 2020 06:25:15 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v10 1/1] ShellPkg/DynamicCommand: add HttpDynamicCommand To: devel@edk2.groups.io, vladimir.olovyannikov@broadcom.com, "Rabeda, Maciej" , Zhichao Gao , Ray Ni Cc: Samer El-Haj-Mahmoud , Jiaxin Wu , Siyuan Fu , Liming Gao , Nd References: <20200902040821.24144-1-vladimir.olovyannikov@broadcom.com> <20200902040821.24144-2-vladimir.olovyannikov@broadcom.com> <30fc1fc2-858f-f0e8-e54f-58b872ad57ae@linux.intel.com> <30233c17-c081-deb6-d0cb-d847c7a9992b@redhat.com> <20ac79e77049e8756de36f50df1e7f36@mail.gmail.com> <8aa008fc-ed91-5118-df37-b24f2eb86c2f@redhat.com> <5534f1de-b9b7-0981-e8c6-de0e4c25c617@redhat.com> <4aa7caa168ad8a2ea48d65722f63f33a@mail.gmail.com> From: "Laszlo Ersek" Message-ID: <95671aa4-df35-91c4-880c-89dadccc011a@redhat.com> Date: Thu, 10 Sep 2020 08:25:14 +0200 MIME-Version: 1.0 In-Reply-To: <4aa7caa168ad8a2ea48d65722f63f33a@mail.gmail.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=lersek@redhat.com X-Mimecast-Spam-Score: 0.004 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Content-Language: en-US Hi Vladimir, On 09/09/20 19:15, Vladimir Olovyannikov via groups.io wrote: >> -----Original Message----- >> From: Laszlo Ersek >> Sent: Wednesday, September 9, 2020 3:51 AM >> To: Vladimir Olovyannikov ; >> Rabeda, Maciej ; devel@edk2.groups.io; >> Zhichao Gao ; Ray Ni >> Cc: Samer El-Haj-Mahmoud ; Jiaxin Wu >> ; Siyuan Fu ; Liming Gao >> ; Nd >> Subject: Re: [PATCH v10 1/1] ShellPkg/DynamicCommand: add >> HttpDynamicCommand >> >> On 09/08/20 23:04, Vladimir Olovyannikov wrote: >>>> -----Original Message----- >>>> From: Laszlo Ersek >>>> Sent: Monday, September 7, 2020 2:37 AM >>>> To: Vladimir Olovyannikov ; >>>> Rabeda, Maciej ; >> devel@edk2.groups.io; >>>> Zhichao Gao ; Ray Ni >>>> Cc: Samer El-Haj-Mahmoud ; Jiaxin >> Wu >>>> ; Siyuan Fu ; Liming Gao >>>> ; Nd >>>> 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