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

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

Kai,
In your case, the two PCD value should be set to 2020/2119.
With the new PCD value the issue you see will disappear.
I won't approve code changes unless you convince me there is a logic error in code.

+ Lijian

Thanks,
Ray

> -----Original Message-----
> From: Yau, KaiX <kaix.yau@intel.com>
> Sent: Wednesday, May 13, 2020 11:37 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,
> 
> Lijian Zhou is also able to reproduce the error. You can ask him for more details if you guys are in the same building.
> 
> 
> 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 PcdMinimalValidYear &
> PcdMaximalValidYear  smaller for testing (such as 1948 and 2047), you can reproduce this issue easily.
> 
>  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: Tuesday, May 12, 2020 11:27 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
> 
> 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|0x
> > > > > 00
> > > > > 00
> > > > > 0
> > > > > 00D
> > > > > +  # @Expression 0x80000001 |
> > > > > gPcAtChipsetPkgTokenSpaceGuid.PcdMinimalValidYear >= 2000
> > > > > +
> > > > > gPcAtChipsetPkgTokenSpaceGuid.PcdMinimalValidYear|2000|UINT16|0x
> > > > > 00
> > > > > 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|0x
> > > > > 00
> > > > > 00
> > > > > 0
> > > > > 00E
> > > > > +
> > > > > gPcAtChipsetPkgTokenSpaceGuid.PcdMaximalValidYear|2099|UINT16|0x
> > > > > 00
> > > > > 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
> > > > >
> > > > >
> > > > >