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....@intel.com>
> Sent: Wednesday, May 13, 2020 11:37 AM
> To: Ni, Ray <ray...@intel.com>; devel@edk2.groups.io; kai...@gmail.com
> Cc: Jiang, Guomin <guomin.ji...@intel.com>; Gao, Liming
> <liming....@intel.com>; Archield, Ralphal
> <ralphal.archi...@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....@intel.com>; devel@edk2.groups.io; kai...@gmail.com
> Cc: Jiang, Guomin <guomin.ji...@intel.com>; Gao, Liming
> <liming....@intel.com>; Archield, Ralphal
> <ralphal.archi...@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....@intel.com>
> > Sent: Tuesday, May 12, 2020 1:52 PM
> > To: Ni, Ray <ray...@intel.com>; devel@edk2.groups.io; kai...@gmail.com
> > Cc: Jiang, Guomin <guomin.ji...@intel.com>; Gao, Liming
> > <liming....@intel.com>; Archield, Ralphal <ralphal.archi...@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....@intel.com>; devel@edk2.groups.io;
> > kai...@gmail.com
> > Cc: Jiang, Guomin <guomin.ji...@intel.com>; Gao, Liming
> > <liming....@intel.com>; Archield, Ralphal <ralphal.archi...@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....@intel.com>
> > > Sent: Tuesday, May 12, 2020 10:02 AM
> > > To: Ni, Ray <ray...@intel.com>; devel@edk2.groups.io;
> > > kai...@gmail.com
> > > Cc: Jiang, Guomin <guomin.ji...@intel.com>; Gao, Liming
> > > <liming....@intel.com>; Archield, Ralphal
> > > <ralphal.archi...@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....@intel.com>; devel@edk2.groups.io;
> > > kai...@gmail.com
> > > Cc: Jiang, Guomin <guomin.ji...@intel.com>; Gao, Liming
> > > <liming....@intel.com>; Archield, Ralphal
> > > <ralphal.archi...@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....@intel.com>
> > > > Sent: Monday, April 27, 2020 1:47 PM
> > > > To: Ni, Ray <ray...@intel.com>; devel@edk2.groups.io;
> > > > kai...@gmail.com
> > > > Cc: Jiang, Guomin <guomin.ji...@intel.com>; Gao, Liming
> > > > <liming....@intel.com>; Archield, Ralphal
> > > > <ralphal.archi...@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; kai...@gmail.com
> > > > Cc: Yau, KaiX <kaix....@intel.com>; Jiang, Guomin
> > > > <guomin.ji...@intel.com>; Gao, Liming <liming....@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....@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....@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
> > > > >
> > > > >
> > > > >
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#59398): https://edk2.groups.io/g/devel/message/59398
Mute This Topic: https://groups.io/mt/72484290/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
