Re: [edk2-devel] [PATCH] PcAtChipsetPkg: Wrong System YEAR displayed in SETUP

["Ni, Ray" <ray.ni@intel.com>]

Kai,
Can you please explain what logic error in code is wrong?

Thanks,
Ray 

> -----Original Message-----
> From: Yau, KaiX <kaix.yau@intel.com>
> Sent: Tuesday, May 12, 2020 1:52 PM
> To: Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io; kaiyau@gmail.com
> Cc: Jiang, Guomin <guomin.jiang@intel.com>; Gao, Liming <liming.gao@intel.com>; Archield, Ralphal
> <ralphal.archield@intel.com>
> Subject: RE: [edk2-devel] [PATCH] PcAtChipsetPkg: Wrong System YEAR displayed in SETUP
> 
> Hi Ray,
> 
> We cannot use 2020 and 2119 if we don't fix the current logic.
> 
> If they keep that code, The PCD values have to be "2000 and 2099" (or 1900 and 1999)
> 
> Any specific reasons why you don't fix the logic? I will need to update the HSD with your reason.
> If we fixed it, we don't need to worry about the PCD values anymore.
> 
> Regards
> Kai Yau
> 
> 
> 
> -----Original Message-----
> From: Ni, Ray
> Sent: Tuesday, May 12, 2020 1:41 AM
> To: Yau, KaiX <kaix.yau@intel.com>; devel@edk2.groups.io; kaiyau@gmail.com
> Cc: Jiang, Guomin <guomin.jiang@intel.com>; Gao, Liming <liming.gao@intel.com>; Archield, Ralphal
> <ralphal.archield@intel.com>
> Subject: RE: [edk2-devel] [PATCH] PcAtChipsetPkg: Wrong System YEAR displayed in SETUP
> 
> I am ok if you change the PCD default value to [2020, 2119].
> 
> Please send out separate patch for the PCD default value change if you want.
> 
> Thanks,
> Ray
> 
> > -----Original Message-----
> > From: Yau, KaiX <kaix.yau@intel.com>
> > Sent: Tuesday, May 12, 2020 10:02 AM
> > To: Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io; kaiyau@gmail.com
> > Cc: Jiang, Guomin <guomin.jiang@intel.com>; Gao, Liming
> > <liming.gao@intel.com>; Archield, Ralphal <ralphal.archield@intel.com>
> > Subject: RE: [edk2-devel] [PATCH] PcAtChipsetPkg: Wrong System YEAR
> > displayed in SETUP
> >
> > Hi,
> >
> > The current default PCD values has issue with the current logic.
> >
> > How do you want to resolve this?
> > If you decide not to fix the logic, at least we need to change the Min value to 2000 and Max value to 2099.
> >
> > If you agreed, you can remove my logic fix (PcRtc.c) from the Gerrit, and just keep the PCD changes and check-in the code.
> >
> > Regards
> > Kai Yau
> >
> >
> >
> >
> >
> >
> >
> > -----Original Message-----
> > From: Ni, Ray
> > Sent: Monday, May 11, 2020 9:52 PM
> > To: Yau, KaiX <kaix.yau@intel.com>; devel@edk2.groups.io;
> > kaiyau@gmail.com
> > Cc: Jiang, Guomin <guomin.jiang@intel.com>; Gao, Liming
> > <liming.gao@intel.com>; Archield, Ralphal <ralphal.archield@intel.com>
> > Subject: RE: [edk2-devel] [PATCH] PcAtChipsetPkg: Wrong System YEAR
> > displayed in SETUP
> >
> > The PCD range (100 years) assumes that:
> > there is no such a x86 system that can use a bios flashed in year 1998
> > and this system will be functional till 2097. Or even it can be functional till 2097, its bios should be flashed multiple
> times during the 100 years.
> >
> > I don't think we need to consider the solution to Y3K because I guess
> > (90% believe) the RTC hardware will disappear after
> > 10 years at most.
> >
> > Thanks,
> > Ray
> >
> > > -----Original Message-----
> > > From: Yau, KaiX <kaix.yau@intel.com>
> > > Sent: Monday, April 27, 2020 1:47 PM
> > > To: Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io;
> > > kaiyau@gmail.com
> > > Cc: Jiang, Guomin <guomin.jiang@intel.com>; Gao, Liming
> > > <liming.gao@intel.com>; Archield, Ralphal
> > > <ralphal.archield@intel.com>
> > > Subject: RE: [edk2-devel] [PATCH] PcAtChipsetPkg: Wrong System YEAR
> > > displayed in SETUP
> > >
> > > Hi Ray,
> > >
> > > The current logic in PcRtc.c will give us WRONG year. (I explained
> > > below) If we only change the PCD Min. value to 2000 and Max. value
> > > to 2099. The current logic will work. But we still
> > have Y3K bug.
> > >
> > > That is why we need to read from Hardware.
> > > If any reasons you guys doesn't want to read from hardware, at least we need to change Min. and Max. value ASAP.
> > > (you can go ahead to delete the change in PcRtc.c and just submit
> > > the PCD changes)
> > >
> > > Regards
> > > Kai Yau
> > >
> > >
> > >
> > > --------------------------------------------------------------------
> > > --
> > > ------- The root cause is from the following logic (PcRtc.c):
> > >
> > > Current Default PcdMinimalValidYear = 1998 Current Default
> > > PcdMaximalValidYear = 2097
> > >
> > > If we can set the YEAR to 2099, we will end up getting 1999 instead.   The reason why user have not found this problem
> > > because Windows does not allow user to set the YEAR to 2099. If we
> > > make the PcdMaximalValidYear  smaller, you can reproduce this issue.
> > >
> > >  Time->Year    = CheckAndConvertBcd8ToDecimal8 ((UINT8) Time->Year);   <-- Time->Year  = 99
> > >   Century = (UINT8) (PcdGet16 (PcdMinimalValidYear) / 100);            <-- Century = 19
> > >   if (Time->Year < PcdGet16 (PcdMinimalValidYear) % 100) {               <-- (99 < 97) = FALSE
> > >     Century++;
> > >   }
> > >   Time->Year = (UINT16) (Century * 100 + Time->Year);                      <-- Time->Year = 1999  (19 x 100 + 99).       Also 1999
> is
> > > within the RANGE, so it is considered as "VALID" data; however it is wrong.
> > >
> > > Regards
> > > Kai
> > >
> > >
> > >
> > >
> > >
> > > -----Original Message-----
> > > From: Ni, Ray
> > > Sent: Monday, April 27, 2020 12:54 AM
> > > To: devel@edk2.groups.io; kaiyau@gmail.com
> > > Cc: Yau, KaiX <kaix.yau@intel.com>; Jiang, Guomin
> > > <guomin.jiang@intel.com>; Gao, Liming <liming.gao@intel.com>
> > > Subject: RE: [edk2-devel] [PATCH] PcAtChipsetPkg: Wrong System YEAR
> > > displayed in SETUP
> > >
> > > This patch introduces several changes for different purposes.
> > > At least I can see:
> > > 1. Default PCD value change
> > > 2. Consume century value from HW
> > >
> > > Can you separate the patches to 2 patches?
> > >
> > >
> > > For the purpose #2, can you explain more background/reason?
> > >
> > > The PcdMinimalValidYear/ PcdMaximalValidYear value are chosen that
> > > when a two-digit year is read from HW, there is no confusion to convert to 4-digit year.
> > > Are you aware of this when you made the patch?
> > >
> > > Thanks,
> > > Ray
> > >
> > >
> > > > -----Original Message-----
> > > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Yau
> > > > Sent: Friday, March 20, 2020 11:42 AM
> > > > To: devel@edk2.groups.io
> > > > Cc: Yau, KaiX <kaix.yau@intel.com>
> > > > Subject: [edk2-devel] [PATCH] PcAtChipsetPkg: Wrong System YEAR
> > > > displayed in SETUP
> > > >
> > > > Function ConvertRtcTimeToEfiTime() will reset to Century 19XX when
> > > > the year is set larger than the value of PcdMaximalValidYear outside SETUP.
> > > >
> > > > Signed-off-by: Yau <kaix.yau@intel.com>
> > > > ---
> > > >  PcAtChipsetPkg/PcAtChipsetPkg.dec             |  7 +++---
> > > >  .../PcatRealTimeClockRuntimeDxe/PcRtc.c       | 25 +++++++++++--------
> > > >  .../PcatRealTimeClockRuntimeDxe/PcRtc.h       | 17 +++++++------
> > > >  3 files changed, 27 insertions(+), 22 deletions(-)
> > > >
> > > > diff --git a/PcAtChipsetPkg/PcAtChipsetPkg.dec
> > > > b/PcAtChipsetPkg/PcAtChipsetPkg.dec
> > > > index 88de5cceea..660cb5d52e 100644
> > > > --- a/PcAtChipsetPkg/PcAtChipsetPkg.dec
> > > > +++ b/PcAtChipsetPkg/PcAtChipsetPkg.dec
> > > > @@ -4,7 +4,7 @@
> > > >  # This package is designed to public interfaces and
> > > > implementation which follows  # PcAt defacto standard.
> > > >  #
> > > > -# Copyright (c) 2009 - 2019, Intel Corporation. All rights
> > > > reserved.<BR>
> > > > +# Copyright (c) 2009 - 2020, Intel Corporation. All rights
> > > > +reserved.<BR>
> > > >  # Copyright (c) 2017, AMD Inc. All rights reserved.<BR>  #  #
> > > > SPDX-License-Identifier: BSD-2-Clause-Patent @@ -61,12 +61,13 @@
> > > >
> > > >    ## This PCD specifies the minimal valid year in RTC.
> > > >    # @Prompt Minimal valid year in RTC.
> > > > -
> > > > gPcAtChipsetPkgTokenSpaceGuid.PcdMinimalValidYear|1998|UINT16|0x00
> > > > 00
> > > > 0
> > > > 00D
> > > > +  # @Expression 0x80000001 |
> > > > gPcAtChipsetPkgTokenSpaceGuid.PcdMinimalValidYear >= 2000
> > > > +
> > > > gPcAtChipsetPkgTokenSpaceGuid.PcdMinimalValidYear|2000|UINT16|0x00
> > > > 00
> > > > 0
> > > > 00D
> > > >
> > > >    ## This PCD specifies the maximal valid year in RTC.
> > > >    # @Prompt Maximal valid year in RTC.
> > > >    # @Expression 0x80000001 |
> > > > gPcAtChipsetPkgTokenSpaceGuid.PcdMaximalValidYear <
> > > > gPcAtChipsetPkgTokenSpaceGuid.PcdMinimalValidYear + 100
> > > > -
> > > > gPcAtChipsetPkgTokenSpaceGuid.PcdMaximalValidYear|2097|UINT16|0x00
> > > > 00
> > > > 0
> > > > 00E
> > > > +
> > > > gPcAtChipsetPkgTokenSpaceGuid.PcdMaximalValidYear|2099|UINT16|0x00
> > > > 00
> > > > 0
> > > > 00E
> > > >
> > > >  [PcdsFixedAtBuild, PcdsPatchableInModule]
> > > >    ## Defines the ACPI register set base address.
> > > > diff --git a/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.c
> > > > b/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.c
> > > > index 52af179417..38a3521dfc 100644
> > > > --- a/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.c
> > > > +++ b/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.c
> > > > @@ -1,7 +1,7 @@
> > > >  /** @file
> > > >    RTC Architectural Protocol GUID as defined in DxeCis 0.96.
> > > >
> > > > -Copyright (c) 2006 - 2018, Intel Corporation. All rights
> > > > reserved.<BR>
> > > > +Copyright (c) 2006 - 2020, Intel Corporation. All rights
> > > > +reserved.<BR>
> > > >  Copyright (c) 2017, AMD Inc. All rights reserved.<BR>
> > > >
> > > >  SPDX-License-Identifier: BSD-2-Clause-Patent @@ -441,8 +441,8 @@
> > > > PcRtcGetTime (  **/  EFI_STATUS  PcRtcSetTime (
> > > > -  IN EFI_TIME                *Time,
> > > > -  IN PC_RTC_MODULE_GLOBALS   *Global
> > > > +  IN     EFI_TIME                *Time,
> > > > +  IN OUT PC_RTC_MODULE_GLOBALS   *Global
> > > >    )
> > > >  {
> > > >    EFI_STATUS      Status;
> > > > @@ -525,6 +525,8 @@ PcRtcSetTime (
> > > >    //
> > > >    if (Global->CenturyRtcAddress != 0) {
> > > >      RtcWrite (Global->CenturyRtcAddress, DecimalToBcd8 ((UINT8)
> > > > (RtcTime.Year / 100)));
> > > > +  } else {
> > > > +    DEBUG ((EFI_D_INFO, "PcRtc: Century RTC Address is not
> > > > + found\n"));
> > > >    }
> > > >
> > > >    ConvertEfiTimeToRtcTime (&RtcTime, RegisterB); @@ -868,6 +870,9
> > > > @@ ConvertRtcTimeToEfiTime (  {
> > > >    BOOLEAN IsPM;
> > > >    UINT8   Century;
> > > > +  UINT8   CenturyRtcAddress;
> > > > +
> > > > +  CenturyRtcAddress = GetCenturyRtcAddress ();
> > > >
> > > >    if ((Time->Hour & 0x80) != 0) {
> > > >      IsPM = TRUE;
> > > > @@ -891,14 +896,12 @@ ConvertRtcTimeToEfiTime (
> > > >      return EFI_INVALID_PARAMETER;
> > > >    }
> > > >
> > > > -  //
> > > > -  // For minimal/maximum year range [1970, 2069],
> > > > -  //   Century is 19 if RTC year >= 70,
> > > > -  //   Century is 20 otherwise.
> > > > -  //
> > > > -  Century = (UINT8) (PcdGet16 (PcdMinimalValidYear) / 100);
> > > > -  if (Time->Year < PcdGet16 (PcdMinimalValidYear) % 100) {
> > > > -    Century++;
> > > > +  if (CenturyRtcAddress != 0) {
> > > > +    Century = CheckAndConvertBcd8ToDecimal8 ((UINT8) (RtcRead
> > > > (CenturyRtcAddress)));
> > > > +  } else if ((PcdGet16 (PcdMinimalValidYear) / 100) == (PcdGet16
> > > > (PcdMaximalValidYear) / 100)) {
> > > > +    Century = (UINT8)(PcdGet16 (PcdMinimalValidYear) / 100);  }
> > > > + else {
> > > > +    Century = RTC_INIT_CENTURY;
> > > >    }
> > > >    Time->Year = (UINT16) (Century * 100 + Time->Year);
> > > >
> > > > diff --git a/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.h
> > > > b/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.h
> > > > index 47293ce44c..94926fe73e 100644
> > > > --- a/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.h
> > > > +++ b/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.h
> > > > @@ -1,7 +1,7 @@
> > > >  /** @file
> > > >    Header file for real time clock driver.
> > > >
> > > > -Copyright (c) 2006 - 2018, Intel Corporation. All rights
> > > > reserved.<BR>
> > > > +Copyright (c) 2006 - 2020, Intel Corporation. All rights
> > > > +reserved.<BR>
> > > >  Copyright (c) 2017, AMD Inc. All rights reserved.<BR>
> > > >
> > > >  SPDX-License-Identifier: BSD-2-Clause-Patent @@ -62,11 +62,12 @@
> > > > extern PC_RTC_MODULE_GLOBALS  mModuleGlobal;  // Date and time
> > > > initial values.
> > > >  // They are used if the RTC values are invalid during driver
> > > > initialization  // -#define RTC_INIT_SECOND 0 -#define
> > > > RTC_INIT_MINUTE
> > > > 0
> > > > -#define RTC_INIT_HOUR   0
> > > > -#define RTC_INIT_DAY    1
> > > > -#define RTC_INIT_MONTH  1
> > > > +#define RTC_INIT_SECOND  0
> > > > +#define RTC_INIT_MINUTE  0
> > > > +#define RTC_INIT_HOUR    0
> > > > +#define RTC_INIT_DAY     1
> > > > +#define RTC_INIT_MONTH   1
> > > > +#define RTC_INIT_CENTURY 20
> > > >
> > > >  #pragma pack(1)
> > > >  //
> > > > @@ -160,8 +161,8 @@ PcRtcInit (
> > > >  **/
> > > >  EFI_STATUS
> > > >  PcRtcSetTime (
> > > > -  IN EFI_TIME               *Time,
> > > > -  IN PC_RTC_MODULE_GLOBALS  *Global
> > > > +  IN     EFI_TIME               *Time,
> > > > +  IN OUT PC_RTC_MODULE_GLOBALS  *Global
> > > >    );
> > > >
> > > >  /**
> > > > --
> > > > 2.17.1.windows.2
> > > >
> > > >
> > > >